Enable unit testing for client libraries

[PureKrome]

As requested by @stankovski and initially asked by @prabirshrestha ...

Create interface/virtual methods so we can easily write unit tests.

eg.

var fakeMsg = new Mock<CloudQueueMessage>();
var cloudQueue = A.Fake<ICloudQueue>();

etc...

[mxa0079]

Started to address this with the following pull request: #90

[PureKrome]

[moswald]

I can't believe we're still waiting on this one.

I'm tired of writing my own wrapper IBlobClient and IBlobContainer objects.

[PureKrome]

@moswald and there is even a PR sent, for this....

[Cheesebaron]

It is amazing that they don't even answer either the PR or this issue...

[mirobers]

First, sorry for the late reply. We have been considering some kind of support for mocking for quite some time, but we want it to be a complete solution that doesn't increase the size and complexity of the codebase. Adding interfaces for our public classes doesn’t meet this requirement; it adds a bunch of files to the codebase that don’t need to be there.

However, we would like to consider an alternative instead: making methods virtual. This will allow easy use of tools such as Moq. We could make all public methods that make service calls virtual, and therefore you will be able to conveniently mock any service call your code makes. Please share your feedback and let us know what you think!

[Cheesebaron]

I think the problem is that you have already overcomplicated the code as is. You have dug yourself very deep into a hole you cannot get out of now, without collapsing the wall on the way out.

[PureKrome]

whoa @Cheesebaron - that escalated quickly!

@mirobers personally, I'm happy with making the method(s) virtual as that fits our mocking requirements.

[moswald]

Making the methods virtual works for me. I understand not wanting to bloat the codebase with interface files that simply mirror the only concrete implementation.

@Cheesebaron, do you have some specific examples on how this library is too complex?

[TimLovellSmith]

@mirobers Virtual methods would be better than nothing - a big step in the right direction - so hooray if you do it! But it can be inconvenient faking out non-interface classes that don't have a default constructor.

[mirobers]

Thanks for the feedback everyone! We will add this to our backlog for a future major version release.

[moswald]

@mirobers If you want to understand exactly what we're looking for, try writing a simple little app that consumes the Azure Storage lib, and then write tests that need to exercise the calls without making any network calls. We use NSubstitute as our mocking framework within xUnit.net tests.

[PureKrome]

@mirobers If I start a PR for this, would that help?

[mirobers]

@moswald The docs for NSubstitute indicate that it supports substituting virtual methods of classes, so you should be good with the planned approach.

@PureKrome Thanks for the offer, but seeing as this change affects many different files across the library we would prefer to do this ourselves to minimize the potential for merge issues.

[TimLovellSmith]

@mirobers I feel concerned at your statement that you don't want substantial contributions because they are hard to merge. Because they are only hard to merge if there are conflicts - and since you can require the pull request to be based on whatever commit you like, you can certainly require the pull requestor to give you a pull request that has no merge conflicts with whatever branch you care about. Is the real reason you can't accept such a pull request actually something else, like your lack of capacity to effectively review/vet large contributions?

[pemari-msft]

@TimLovellSmith
I appreciate your concern. We do understand the merging process – as you might imagine, there’s active, ongoing development for this library happening behind the scenes. Because of that, it is nontrivial to ensure a pull requestor will not have merge conflicts (and this will be a sweeping change touching many files, increasing the likelihood of conflicts) as they will not have access to the branches where development has occurred. We also follow the semver versioning scheme, and so, as this is a larger change, we are looking to include this in a regular release milestone.

So, while we have the capacity to review large contributions via github, it only makes sense to accept them if the effort for the change is outweighed by the effort of taking the change from the community. Hope this helps clarify!

[Cheesebaron]

as they will not have access to the branches where development has occurred

Can you explain why this is so?

[PureKrome]

@Cheesebaron My guess is that an MS dev has made a local branch on their localhost machine and not pushed it up -- so we can't see/access that dev-branch.

. * * assumption **

[Cheesebaron]

My guess is that they are actively developing on a git repository internally and when they feel like it (i.e. they have something major to release) they push changes to github. Which is super bad practice if you want to accept pull requests.

[guardrex]

Using TFS internally would make it more difficult.

Off-topic slightly, so I hope the OP doesn't mind if I interject here ... Some of the management at MS seems like they are either resistant to the community involvement concept or just waiting to see how it plays out with .NET and CoreFX before they embrace it. I think its working great over there, and this will probably be the smoothest release of the strongest .NET ever, because so many issues have been filed and addressed based on community members testing of nightly builds and prereleases and because many PR's in CoreFX have been for optimizations.

I hope the Storage Team is able to accept more PR's in the future and accept more good suggestions from the community. I still think we need an Annoucements repo!

IMHO @PureKrome should have been allowed to take a shot at the PR. If the team likes it and the unit tests pass, hey! better for all of us. If the team has problems and/or unit tests blow up, then I'm sure @PureKrome would understand if it were rejected. Shooting it down because it would "potential[ly]" not work out is a really bad way to continue developing software at MS ... again IMHO. At least look at the PR, then decide.

[mirobers]

Thanks for the feedback everyone. This is getting really off-topic, but I want to make sure everyone knows the state of community contributions on the Azure storage repos. Our official policy is that we do accept community contributions in our repos. The newer storage repos (like the python client, iOS client, data movement library) are set up with contributions in mind and we have seen that model work well. We would like to make contributions easier in all of our repos and we are considering ways to restructure things internally to facilitate this.

In this particular case, at this particular time, unfortunately the work required to accept the contribution would outweigh the effort saved. I would not want anyone to spend their time on a PR that ultimately will not speed up the process.

Finally, let me restate that we are aware of the barriers to accepting contributions in this repo and we are working on removing those barriers. Of course, the Azure SDK Contribution Guidelines will continue to apply and we expect and appreciate coordination with us when contributing code.

[pemari-msft]

Hi all -- as you requested, we have unsealed all classes and virtualized all methods making service calls. We've verified that this enables mocking scenarios. Thanks for the request!

[PureKrome]

[flyingUnderTheRadar]

Did I miss something. It does seem that we still have those classes as sealed. Example

azure-storage-net/Lib/WindowsRuntime/Queue/CloudQueueMessage.cs

Line 25 in 81cb4ae

public sealed partial class CloudQueueMessage

[joshrizzo]

Did I miss something. It does seem that we still have those classes as sealed. Example

azure-storage-net/Lib/WindowsRuntime/Queue/CloudQueueMessage.cs

Line 25 in 81cb4ae

public sealed partial class CloudQueueMessage

Ya, CloudQueue is still not mockable... We seems to have made backwards progress