Support for dependency injection with interfaces for testing purposes

[Dongata]

Can you please extract the signatures of the admin classes into services so we can mock them to test our code

Thank you

[hiranya911]

@Dongata can you elaborate what exactly you mean by services?

In general, this is the pattern I'd recommend for developers (taking FirebaseMessaging API as an example:

interface INotificationSender {
  Task sendAsync(String token, String title, String body);
}

class FirebaseNotificationSender : INotificationSender {
  // Implement the interface using `FirebaseMessaging` class from the SDK.
}

class MockNotificationSender : INotificationSender {
  // Mock implementation for testing
}

Then you use the INotificationSender interface everywhere in your code, where you wish to send a notification. This cleanly separates Firebase SDK from your code, and makes it easier to test. In the future if you choose to change how you send notifications (e.g. say using our REST API), that just becomes a matter of coming up with a new implementation for the same interface.

[Dongata]

Yeah, we're using it like that, But i think that will be great if that the inversion of control, can be supported by default on the library, given that is like a netcore standard.

Thanks for your help tho.

[hiranya911]

Most of the time defining such interfaces in the SDK is counterproductive. Each user only cares about a subset of the SDK methods, which means one of 2 things can happen:

1. If we extract all signatures of a class into a single interface (e.g. FirebaseAuth --> IFirebaseAuth) users will end up having to implement very large interfaces, where most methods are of no interest to them.
2. Alternatively we can extract each existing class into a number of small interfaces. But this also leads to several issues such as:
   • It's a major diversion from the conventions used in our client SDKs and our existing Admin SDKs.
   • It's not clear where we should draw the lines when it comes to splitting interfaces. We certainly don't want to inundate the SDK with a large number of interfaces.

Personally I think asking developers to define their own interfaces is the best course of action. Since developers know best about their specific application requirements, they are in the best position to decide what these interfaces should look like.

I will keep this issue open for a bit longer to see if it receives any additional feedback/suggestions.

[ghost]

@hiranya911 The solution you propose merely shifts the problem:
It is impossible to test the class FirebaseNotificationSender.
There is no alternative to being able to mock a service.
At one point or another, there will be an untested service.

Please re-evaluate this design decision, as it introduced great risk at critical services, interfacing with FirebaseAuth and FirebaseMessaging, for example.

Mocking can be supported by offering non-sealed classes using Moq.

And please rename this issue, its name is very misleading:
This has nothing to do with IoT, but is simple adding support for Mocking.
Adding IoC support for the entire SDK would be a lot more work.
I seemingly added a duplicate due to this misleading name and my ticket has more information, so I will leave it up to you to merge these, if that is okay
#196

[Dongata]

Yeah you right about the title, english is not my first language c:

Mocking those services is not imposible, is just annoying. i'll pass you an example on your issue, hope it helps

[hiranya911]

It is impossible to test the class FirebaseNotificationSender.

Assuming that class simply turns around and calls the FirebaseMessaging API, you shouldn't need to test it. That's the idea.

Mocking is just a way to swap out third-party or otherwise unmodifiable code with your own code for testing. The solution above has the same net result. You just treat INotificationSender as the main programming interface, and FirebaseNotificationSender as an unmodifiable implementation of it. Then MockNotificationSender just becomes your mock for FirebaseNotificationSender. FirebaseNotificationSender can still participate in any end-to-end tests executed against a test Firebase project.

Mocking can be supported by offering non-sealed classes using Moq.

If we were to support direct mocking of SDK interfaces, I'd like to think through different options and the developer experience a bit more before we start unsealing our APIs. There are other classes like UserRecord, FirebaseToken and BatchResponse we need to think about as well. Which APIs are you looking to mock, and what would your ideal test code look like?

[hiranya911]

Also related to #158

[ghost]

By introducing interfaces, the developer receives more flexibility and a cleaner public interface.
Yes, I can write an adapter (copy pasting your classes public signatures), but this will be prone to errors when you upgrade your SDK.
Interfaces provide a stable public API to work against, while the internals of the class can change at any time.

Unsealing classes simply enables developers to extend your classes and add extra code they require (and also add Mocks, if you have no interfaces).
Yes, developers can put stones in their way, but you typically only extend a frameworks classes if you have special requirements, e.g. due to standards and regulatory guidelines in specific fields.

Of course mocking of services would also require the possibility to create new BatchResponses using a public constructor. As this is just a data object, I see no reason for an internal constructor.
Your documentation clearly states to use the service directly and BatchResponse also has xmldoc communicating that this is a response object.

Simplified example of mocking and testing a service, which uses a firebases service (e.g. verifying that we treat the response objects correctly)

//TODO impossible to Mock with FirebaseAdmin 1.13.0 (sealed class without interface)
var firebaseMessagingMock = new Mock<FirebaseMessaging>();
firebaseMessagingMock
	.Setup(messaging => messaging.SendMulticastAsync(It.IsAny<MulticastMessage>()))
	.ReturnsAsync(() =>
		new BatchResponse(new[]
		{
			SendResponse.FromMessageId("messageId1"),
			SendResponse.FromMessageId("messageId2")
		}));
var firebaseService = new FirebaseService(otherServiceMock.Object, firebaseMessagingMock.Object, Bouncer.Instance,
	NullLogger<FirebaseService>.Instance);
// TODO test firebaseServiceBehaviour

[hiranya911]

Yes, I can write an adapter (copy pasting your classes public signatures), but this will be prone to errors when you upgrade your SDK.

Couple of points worth noting here:

1. You don't necessarily have to mimic our API surface. In fact, in most cases you shouldn't (see my comment below about wrappers).
2. Our public API is already stable with strict adherence to SemVer. So you won't encounter breaking API changes unless we bump the major version number. In which case, you have to update some of your code to match any way.

Unsealing classes simply enables developers to extend your classes and add extra code they require (and also add Mocks, if you have no interfaces).

I'm not against making it possible to mock certain APIs. But if we do that I'd like to do it in a SDK-wide, consistent way (or at least have a plan to make it so). Focusing on 1-2 classes at a time is likely not going to work out well in the long run.

Simplified example of mocking and testing a service, which uses a firebases service (e.g. verifying that we treat the response objects correctly)

What is FirebaseService in this example? Is that some class in your code or something provided by the SDK?

It also seems this is simple enough to handle via some wrappers in your code (at least as a stop gap measure).

public class NotificationResponse {

}

public class NotificationSender {
    public async Task<NotificationResponse> SendMulticastAsync(MulticastMessage message) {
        // Impl using FirebaseMessaging
    }
}

Now you can mock NotificationSender for unit testing. I understand it's more work, but it gets the job done at least until we make the necessary changes in the SDK-end. There are also schools of thought that argue that this is in fact the best practice for consuming external dependencies :)

[MKaranusic]

I ran into the same issue trying to mock BatchResponse in my unit tests.

I found the same issue submitted on firebase-admin-java repo
where BatchResponse was turned into an interface.

This is the PR that soloved the issue on java repo.

This would make writing unit tests around the api much simpler for our c# projects.
Thank you.

[Dongata]

If someone has this same issue, here's a work around
#196 (comment)

[aschwenker-insight]

Bumping this as my team is also running into this issue.

This all seems to be a matter of opinion with one camp saying, "You should always use a pure adapter" and the other saying, "Make an interface for the public methods and properties." Both of which are valid approaches.

Pure Adapter

If you are using a pure adapter, you're creating a class and interface that directly duplicates functionality like:

public class MyMessage
{
    // Wrapper around FirebaseAdmin.Messaging.Message

    public Message ToMessage()
    {
        // Conversion code for FirebaseAdmin.Messaging.Message
    }
}

public interface IMyFirebaseService 
{
    public Task<string> SendMessageAsync(MyMessage message);
}

public class MyFirebaseService : IMyFirebaseService
{
    public async Task<string> SendMessageAsync(MyMessage message)
    {
        var firebaseApp = FirebaseApp.GetInstance("Firebase");
        var firebaseMessaging = FirebaseMessaging.GetMessaging(firebaseApp);
        return await firebaseMessaging.SendAsync(message.ToMessage());
    }
}

And using it like:

public class MyNotificationService
{
    private readonly IMyFirebaseService _myFirebaseService;

    public MyNotificationService(IMyFirebaseService myFirebaseService)
    {
        _myFirebaseService = myFirebaseService;
    }

    public async Task SendNotificationAsync(MyMessage message)
    {        
        var result = await _myFirebaseService.SendMessageAsync(message);

        // Do something based on result
    }
}

In this case, you treat MyFirebaseService as a black box because it's an adapter, its implementation details are specific to Firebase, and its implementation will change if you change notification services. This means you mock IMyFirebaseService in MyNotificationService and only test MyNotificationService. The downside is you don't unit test MyFirebaseService.SendMessageAsync(Message) and instead verify it works through integration tests. This hurts your unit test code coverage and means you must be very careful to limit the amount of logic in MyFirebaseService to keep from inadvertently adding bugs.

Dependency Injection Adapter

With a Dependency Injection adapter, FirebaseApp and FirebaseMessaging would implement interfaces and the code above becomes something like:

public interface IMyNotificationService 
{
    public Task<string> SendMessageAsync(Message message);
}

public class MyNotificationService : IMyNotificationService
{
    private readonly IFirebaseApp _firebaseApp;
    private readonly IFirebaseMessaging _firebaseMessaging;

    public MyNotificationService(IFirebaseApp firebaseApp)
    {
        _firebaseApp = firebaseApp;
        _firebaseMessaging = _firebaseApp.GetMessaging();
    }

    public async Task SendMessageAsync(string title)
    {
        var message = new Message
        {
            // Implementation details
            Title = title
        };

        var result = await _firebaseMessaging.SendAsync(message);

        // Do something with result
    }
}

In this case, MyFirebaseService isn't required because IFirebaseApp is injected into the MyNotificationService constructor directly. You mock IFirebaseApp and IFirebaseMessaging and configure IFirebaseApp.GetMessaging() to return your mocked IFirebaseMessaging instance. You can then test MyNotificationService.SendMessageAsync(string) directly. All the boilerplate code from MyFirebaseService disappears.

The downside here is the need to mock IFirebaseApp and IFirebaseMessaging, but mocking libraries like Moq can handle that directly without the need to implement the entire IFirebaseApp and IFirebaseMessaging interface in your own class. There is never a need for an SDK user to implement IFirebaseApp or IFirebaseMessaging directly because they are using the mocking library or the Firebase Admin SDK instances.

The other downside is the MyNotificationService code is also tied to Firebase, which makes switching notification providers more difficult.

Opinion

The point of pure adapters is to hide implementation details from callers of the adapter, require loose coupling, and make swapping components easier because the business logic is separated from implementation details.

However, pure adapters also require creating (at least some) boilerplate code and (potentially) extra classes/interfaces to wrap the adapted code, its exceptions, and its data structures. When the adapted code changes either from moving to the next major version or changing the underlying provider, the adapter's code must also change. Those changes typically cascade up the call stack when our abstractions must change.

A point that is hammered into our heads when learning Object-Oriented Programming and reading books like Clean Code is the need for flexibility and loose coupling in our designs. In fact, that is why the adapter pattern exists. The quest for this ideal often leads to over-engineered and complex code with hidden technical debt where our adapter abstractions still end up being too tightly coupled and the need to change the providers used by our adapters does not happen frequently enough to justify the extra work of creating the adapter in the first place.

Instead, the adapter acts as a warm blanket so we can sleep easier at night knowing that if we had to change providers, we could. But we probably won't, or, at least, won't change if often enough that the effort to create a pure adapter pays off.

Implementing interfaces for the public functionality in the Firebase Admin SDK still allows users who want to create a pure adapter to continue to do so. It also allows users who want to leverage Dependency Injection and Inversion of Control to do so without needing to resort to Fakes or Shims.

[KUTlime]

+1 here. Same problems - SDK is untestable out of the box. 👎🏿

[benkins]

+1

[vidp1]

The amount of wrappers needed to crate a BatchResponse that can be mocked is still unreasonable. At least the response composition should be done with interfaces. So that when you create an SDK wrapper you can mock the responses. And your wrapper contains no logic beside proxying.