Extract interface for ManagementClient to support mocking

[smithderekm]

Currently the ManagementClient does not implement an interface, and thus cannot be mocked for unit test purposes.

Proposing to extract IManagementClient with public methods so that developers who may utilize the management client in their own code may mock interaction with the Azure environment.

[SeanFeldman]

@smithderekm could you please describe what your test is doing? I have a few thoughts on the topic, but it would be wrong to share it before having a better understanding what exactly you're trying to do. Thanks.

[smithderekm]

@SeanFeldman essentially it is a case where I have a wrapper class (think repository pattern) around the management client where I may be doing my own logic before or after calling a ManagementClient function. I want to test my code, but because the ManagementClient cannot be mocked, my unit test turns in to an integration test requiring actual access to an Azure Service Bus namespace.

Here's a simple example of a function I would want to unit test

// assuming
this.client = new ManagementClient(connectionStringBuilder);
            
public async Task<bool> SubscriptionExistsAsync(string topicPath, string subscriptionName)
    {
        this.logger.LogInformation($"Checking for subscription {subscriptionName} on topic {topicPath}");

        // call Service Bus Management Client
        var found = await this.client.SubscriptionExistsAsync(topicPath, subscriptionName);

        this.logger.LogInformation($"Subscription {subscriptionName} {(found ? "was" : "was not")} found on topic {topicPath}");

        return found;
    }

I believe simply extracting the Interface for the ManagementClient public functions should be enough, though obviously would confirm with various mocking frameworks to see if any other changes are required.

I'm interested in your thoughts or suggestions.

[SeanFeldman]

Thanks @smithderekm. Looking at this code above, I'm wondering if the code would work let's say against file system, would you do something like File.Exists()? Isn't that what BCL promises you it would work? And same with ASB, you do not need to test the management client. But, the difference with the management client is that it is not as predictable as file system.

I've done similar type of tests in the past and here are my learning:

1. If you do something with ASB, you don't want to mock. You want to test against the real thing to run into all possible issues.
2. You can create an adapter and test using your adapter w/o the need in an interface.

From the library point of view, an interface could be unnecessary. There's only one implementation of the management client. Note that the client is not sealed or static. You can extend it and create your test double if needed. But if you manage to come up with a stronger reason to provide something for testing, I would rather see methods marked as virtual rather than having to manage an interface and keep it for a single implementation.

So for now, you can:

1. Create a wrapper.
2. Do test-doubles.

Would that work?

[SeanFeldman]

@smithderekm ping

[SeanFeldman]

@smithderekm could you please review #631 (comment) and reply? Thanks 🙂

[smithderekm]

@SeanFeldman sorry for going dark.

I'm going to argue the difference in the case of the file system (again, from a unit testing perspective) is that I can quite easily set up my test runner to also deploy files to an expected location in order to ensure my test runs. In that case, I am not testing if File.Exists() works - I assume it works - but I am controlling the environment in which it is operating.

Contrast that to the Management Client - where in order to do the equivalent of 'deploying a known file' I have to set up a tenant, configure it, etc. Much higher degree of complexity that a simple interface would allow me to avoid.

I notice there are interfaces on the other clients (IQueueClient, ITopicClient, ISubscriptionClient) and there are only single implementations in the SDK. So I'm unclear as to why taking a similar approach to the Management Client is a departure or creates a maintenance burden.

I have a PR I can refresh and submit if appropriate. Just let me know.

[SeanFeldman]

@smithderekm having a contract for a single implementation to be able to mock during testing phase is something I don't like about how we do things in C#. By defining methods virtual the same would be achieved, w/o the noise of an unnecessary contract. If that works for you, I think that would be a better approach.

[SeanFeldman]

Note: adding virtual would be considered a breaking change.