Azure-functions-durable-extension: Create interfaces for some classes to allow unit testing.

[SimonLuckenuik]

Hi,

The following classes would benefit from implementing an interface: Microsoft.Azure.WebJobs.DurableOrchestrationClient, Microsoft.Azure.WebJobs.DurableOrchestrationContext, Microsoft.Azure.WebJobs.DurableActivityContext

There is no way currently that I can unit test any piece of code using those classes, considering that the ctor is also internal. Creating a simple IDurableOrchestrationClient would enable us to test properly. Currently, I am forced to create a decorator that copies all the public methods and wrappes the instance in order to do unit testing.

Thanks!
Simon

[cgillum]

I'm personally not a big fan of interfaces because of the versioning challenges they create. Would abstract base classes work for you or does your unit testing mechanism require interfaces?

[SimonLuckenuik]

I do not understand how they create versioning challenge, considering that you currently expose the full class anyway, and that you would need to prevent breaking changes. Could you elaborate on that?
In theory, if your interface is aligned with your class public members implementation, it should not be an issue.

Abstract classes would involve having most your methods virtual, which in my view makes it more complicated. And considering that those classes are currently sealed I believe it would break you design for the wrong reason.

The goal here would be to allow us to choose to rely either on concrete classes or interfaces for dependency injection.

[cgillum]

The versioning challenge I was referring to is that if we want to add a new method to an existing interface sometime in the future, that will be a breaking change for any existing implementations (including yours) since all implementations are required to implement all methods. Abstract classes don't have this problem because they allow us to create a default implementations and can therefore be versioned more easily. This is based on my experience working in large .NET framework libraries.

Either way, this is a reasonable request and we'll look into it.

[SimonLuckenuik]

Excellent!
Considering that we are using mocking of interfaces, I guess it won't be an issue in our case.

You could also consider having different interfaces with less responsibilities which would also minimize the impact of the issue you are referring to.

[cgillum]

This has been resolved using abstract base classes. See the linked PR for details. See the new sample test project for an example of how to unit test an orchestrator function using Moq: https://github.com/Azure/azure-functions-durable-extension/blob/dev/samples/VSSample.Tests/HelloSequenceTests.cs

Thanks to @gled4er for this contribution!

[gled4er]

@cgillum Thank you very much for the great guidance and collaboration on this issue!

[synesthesia]

Fantastic news @cgillum and @gled4er

I'm probably being really stupid, but am I correct in saying that I can't use this until a new Nuget release, unless I rewrite my project to depend on a local copy of this repo rather than the Nuget?

[gled4er]

Hello @synesthesia ,

Thank you for the comment!

You are right, the feature is not release yet.

@cgillum can you share when the feature will be released?

Thank you!

[synesthesia]

Yes I was having a "duh" moment. @gled4er :-)

have had to fork anyway as I want to abstract a base class for DurableActivityContext too

[gled4er]

@synesthesia This is interesting. May I ask you to explain more about your use case and the need to abstract DurableActivityContext ? We were discussing about abstracting it too but we assumed that most of the time this will not be required.

[synesthesia]

@gled4er I started off by adapting the patterns shown by @devkimchi in this repository. (bear in mind I started my code before you guys had added the testability pull request.)

So the activity functions all look a bit like this:

    public static class MyActivities
    {
        /// <summary>
        /// Gets or sets the <see cref="Factories.FunctionFactory"/> instance.
        /// </summary>
        public static FunctionFactory FunctionFactory { get; set; } = new FunctionFactory();

        [FunctionName("ActivityXrmGetExportParties")]
        public static async Task<ListRequestParties> GetExportParties([ActivityTrigger] DurableActivityContext context, ILogger log)
        {

            var result = await FunctionFactory.Create<IXrmGetExportPartiesFunction>(log)
                                              .InvokeAsync<XrmGetExportPartiesOptions>(context)
                                              .ConfigureAwait(false);


            return result;
        }
    }

And the inner implementation functions all look a bit like this:

public class XrmGetExportPartiesFunction: ActivityFunctionBase<ListRequestParties>, IXrmGetExportPartiesFunction
    {
        private readonly IXrmService _xrm;

        public XrmGetExportPartiesFunction(IXrmService xrm)
        {
            _xrm = xrm ?? throw new ArgumentNullException(nameof(xrm));
        }

        public override Task<ListRequestParties> InvokeAsync<TOptions>(DurableActivityContextBase context, TOptions options = default(TOptions))
        {
            var exportRequest = context.GetInput<ListExportRequest>();
            Log.LogInformation($"Retrieving activity parties for export request Correlation = {exportRequest.Correlation}.");
            // TODO replace with call to external service
            return Task.FromResult<ListRequestParties>(new ListRequestParties());
        }
    }
}

the base looks like:

public abstract class ActivityFunctionBase<TResult> : IActivityFunction<TResult>
{
    // omitted for clarity
}

public interface IActivityFunction<TResult> : IFunction
    {
        Task<TResult> InvokeAsync<TOptions>(DurableActivityContextBase context, TOptions options = default(TOptions));
    }

Your question made me think a bit - I probably don't need to pass the DurableActivityContext in to my inner implementation class. I could instead just use the generic options parameter. The outer function (the entry point from the runtime) will still have to map arguments into the inner function, but that will be unit testable if I change the signature to just take as an argument a class from my model.

I'm going to keep the idea of a function factory instantiating the inner implementation as that lets me use Dependency Injection, but I think you are right to suggest that a base class for DurableActivityContext isn't needed.

[gled4er]

@synesthesia Thank you very much for the detailed explanation!

Let us know if you find some reasons for abstracting DurableActivityContext.

[SimonLuckenuik]

If you want to enable full unit testing for us, we require any concrete class injected inside the Function method to be mockable or instantiable with all required parameters. Currently, I created decorators for all things which are not mockable and created a custom interface representing the concrete class in order to do unit testing, which is pain. That approach also means that I cannot directly write my function code inside the static method, because I need an intermediary layer that will create the decorators in order to inject them in my actual function implementation through a factory supporting DI, where I can inject the contextes or anything provided to the Function static function, something similar to what @synesthesia has shown.

But, I still don't understand why doing base classes instead of using interfaces, which is way simpler for everyone in my view. If using any modern mock library (and even not so modern mock libraries), adding a method to an interface will not impact my UT.

Just to give interfaces examples, this is what Azure Functions is doing in general, this is standard stuff in the examples:
ITable: https://docs.microsoft.com/en-us/azure/azure-functions/functions-bindings-external-table
ICollector: https://docs.microsoft.com/en-us/azure/azure-functions/functions-bindings-storage-table#output---c-script-example
IAsyncCollection: https://docs.microsoft.com/en-us/azure/azure-functions/functions-bindings-notification-hubs#example---apns-native

You can prevent that being a breaking change by configuring your DI to support both IDurableActivityContext and DurableActivityContext injection, so developers can continue working as before and those interested in being more professional and test their stuff properly can use the interface injection :-)

Even currently design wise, having all your methods being virtual to enable developers to unit test is not a good practice, because it's not clear for a developer trying to specialize the class what behavior of the class is OK to modify and override. I will refer to this post from Eric Lippert ( which I believe is a good reference for C# stuff ;-) ): https://stackoverflow.com/a/14451437

[gled4er]

Hello @SimonLuckenuik ,

Thank you for your comment and detailed feedback.

Our main concern is that using interfaces that most probably will be extended with new methods in the future will force users to change their code which is something we want to avoid.

In the same time the abstract classes give us better chance to provide implementation for certain methods. For example we can add implementation of core methods that are referenced by overloads.

[SimonLuckenuik]

Thanks for the answer and clarification on your intent @gled4er.

I know my comments are picky, but my goal is to make sure that this framework usage is aligned with the other parts of Azure Functions in general to make sure that Framework users will have a similar unit testing and API usage between the main Azure Functions library and this extension framework.

So my last comments, I will not spend any more effort trying to convince after those, I understand the change is already done and that my implication in this project is limited to making design comments :-)

I don't see how adding methods to an interface will force a user to change his code:

• For application code, if the method is new, it is not used by the existing code, so no impact here for sure.
• For unit tests, most developers these days are using runtime Mocks which implement the interface dynamically (Moq or any other), which means that those new added methods will be taken care of by the Mock engine (implemented dynamically at runtime for you), so no code change and no broken compilation here as well. For example, the tests in HelloSequenceTests.cs would be written the exact same way with an interface, and if you add an overload for CallActivityAsync to the interface, your test would still be the exact same and everything would compile correctly.

Those misc context classes are managed internally by the framework and injected to the Function code, the framework developers have full control over it, and there are no chances that anyone outside people committing in this repository will ever need to modify or extend that class. So those virtual methods / abstract classes should only be added if they have added value to the framework team internally, not for users of the framework and not for framework user unit testing purpose.

Thanks :-)
Simon

[SimonLuckenuik]

Concerning @synesthesia comment on DurableActivityContext class, we currently have no way to instantiate that data container class from a unit test to inject it, the constructor is internal.

Other not related comment, the class is not sealed like DurableOrchestrationContext, but should probably be sealed to be aligned since we don't have any reason to specialize it.

[cgillum]

Simon, I appreciate your thoughts and care on this topic.

You mentioned an insightful post by Eric Lippert - I think what we're doing matches his advice perfectly fine: we're making methods virtual with a very specific intent - mocking for unit tests.

Azure Functions uses interfaces elsewhere as you mentioned, but those are legacy to the WebJobs SDK and intended primarily for binding extensibility, which is a very different use case vs mocking/unit testing. These interfaces are also a source of conflict within the Functions team because of the compatibility issues they create as we try to evolve the core framework. You can get a sense for some such issues here: https://github.com/Azure/azure-webjobs-sdk/issues/921. My intention was to avoid such problems by starting with abstract base classes. Given that this is a separately-managed extension of Functions/WebJobs and not part of the core, I'm less concerned about the lack of consistency.

It's good to know that modern mock frameworks are not directly impacted by interface changes, but we have no way of guaranteeing that all consumers are using a modern mocking framework. We are responsible for ensuring the compatibility of all potential use cases and we take this responsibility very seriously. We cannot assume that developers will do things "the right way" because invariably there are those who don't and historically we've have to bend over backwards to accomodate them at the last minute. Abstract base classes gives us much greater flexibility and protection in this regard.

Just to be clear, is the concern primarily about consistency within the WebJobs/Functions ecosystem or are there also issues that prevent you from effectively authoring unit tests?

[SimonLuckenuik]

The change does fill the need of unit testing, if I can mock/specialize that class for unit testing. So the remaining concern was really about the consistency/good practices, so this is why I will not insist more :-)

I believe the issue 921 you are referring to, the problem is not the fact that they are using interfaces, but the fact that the interface responsibility wasn't implemented the same way for all extension frameworks (or the responsibility wasn't properly defined, so not always implemented the same way for all extensions).