twilio-csharp
twilio-csharp copied to clipboard
twilio
API feedback: Make classes derive from interfaces so they're mockable in testing
5.0.0-rc4 api/taskrouter/v1/workspace
I'd like to test some logic that interacts with Twilio, but I'm not able to mock classes like WorkerResource since they either (a) don't derive from an interface or (b) don't have overridable methods (like GetDateStatusChanged). Option (a) is much preferred.
Also, for POCO classes, I don't necessary think the classes need to be derived from an interface as long as they can be strongly instantiated with all their properties. The classes as they exist now though can't be instantiated, so I'm forced to create an anon object while referencing the API and then passing the serialized JSON for that object into the .FromJson method. That's definitely not preferred.
Adding interfaces for every resource in the API would add a lot of bloat to the library and is not functionally necessary. I would say the better course of action for testing is to mock whatever wrapper class you create for interacting with WorkspaceResource and the rest of the C# sdk and test against those.
On the POCO instantiation bit, yea I think we can do something about that.
@amattie We are planning on looking at doing this. In the meantime, here's a way to instantiate the POCO yourself, initializing it from the values of an anonymous object: https://github.com/TwilioDevEd/task-router-csharp/blob/master/TaskRouter.Web.Tests/App_Start/WorkspaceConfigTest.cs#L14-L17
And example of using it: https://github.com/TwilioDevEd/task-router-csharp/blob/master/TaskRouter.Web.Tests/App_Start/WorkspaceConfigTest.cs#L55-L61
I get the not wanting to 'add interfaces to all the things' sentiment, but I was very disappointed to find that everything is static now... that doesn't fit in with the ASP.NET Core approach at all. Dependency injection, options/configuration, etc. I especially find the static configuration of the credentials to be very unexpected -- not because we need more than one set of credentials ourselves, but because I could easily see scenarios that would.
I'd been waiting for v5 for a long time but I think now I'm just going to bite the bullet and roll my own client using HttpClient. No hard feelings or anything like that, it's just not what I expected.
@tuespetre Understood. Improvements are on the way, but also, you can inject your own ITwilioRestClient instead of going the static route of credential intialization. Here's an example I put together showing how to mock your own client and fake Twilio REST API responses:
https://github.com/dprothero/twilio-mock-example/blob/master/UnitTestProject1/UnitTest1.cs
Also, why roll your own client from scratch? The library has taken care of all kinds of details for you (paging, authentication, error response parsing, etc.) Perhaps a better route would be to wrap the library with your own wrapper. It's not a bad practice anyway. Third-party dependencies are usually wrapped, at least thinly, by a layer of abstraction.
If there's a scenario you can't see implementing with the new library, let me know and I'll see what I can do to help. And, of course, if you need help making the raw API requests with HttpClient, I can help with that as well.
@dprothero The new library is an improvement over the prior version with most respects, but not having interfaces and having everything static is definitely a step backwards.
I agree with @carlosdp with regards to the interfaces. Wrapping the Twilio client and binding an interface to that seems very reasonable to me.
On the other hand I have to agree with @tuespetre & @waynebrantley on the static issue. This is especially painful when it comes to credentials, as previously noted. Trying to switch between sub-accounts in the same app domain (an MVC app for instance), can not be done safely, and has been a non-starter for us.
This issue has been added to our internal backlog to be prioritized. Pull requests and +1s on the issue summary will help it move up the backlog.
