The Wayback Machine - https://web.archive.org/web/20201202225610/https://github.com/twilio/twilio-csharp/issues/268
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API feedback: Make classes derive from interfaces so they're mockable in testing #268

Open
amattie opened this issue Jul 27, 2016 · 9 comments
Open

Comments

@amattie
Copy link

@amattie amattie commented Jul 27, 2016

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.

@amattie
Copy link
Author

@amattie amattie commented Jul 27, 2016

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.

@carlosdp
Copy link
Contributor

@carlosdp carlosdp commented Aug 1, 2016

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.

@dprothero
Copy link
Contributor

@dprothero dprothero commented Mar 9, 2017

@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

@tuespetre
Copy link

@tuespetre tuespetre commented Mar 24, 2017

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.

@dprothero
Copy link
Contributor

@dprothero dprothero commented Mar 24, 2017

@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.

@waynebrantley
Copy link

@waynebrantley waynebrantley commented May 18, 2017

@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.

@jrmitch120
Copy link

@jrmitch120 jrmitch120 commented Sep 6, 2017

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.

@childish-sambino
Copy link
Contributor

@childish-sambino childish-sambino commented Aug 3, 2020

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.

@gagandeepp
Copy link

@gagandeepp gagandeepp commented Sep 15, 2020

Interested and willing to contribute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.