kontent-ai / delivery-sdk-net

Kontent.ai Delivery .NET SDK
https://www.nuget.org/packages/Kontent.Ai.Delivery
MIT License
32 stars 42 forks source link

Change IDeliveryClient to make faking responses simpler #216

Closed robertgregorywest closed 4 years ago

robertgregorywest commented 4 years ago

Motivation

In order to fake responses from the IDeliveryClient you currently have to mock the HttpClient and set up the desired json as set out in the Faking responses wiki page. This feels like more effort than should be required, and involves too much Kontent infrastructure in my unit tests. Ideally you should be able to do something like this:

var deliveryClient = A.Fake<IDeliveryClient>();
var deliveryItemResponse = A.Fake<DeliveryItemResponse<Article>>();
A.CallTo(deliveryClient).WithReturnType<DeliveryItemResponse<Article>>().Returns(deliveryItemResponse);

However, this is not possible because the constructor for the DeliveryItemResponse is internal, so the above will throw an exception.

Proposed solution

A quick solution would be to make the relevant constructors public but that feels like a bad design choice. I propose the creation of an IDeliveryItemResponse interface which should be used as the return type for the IDeliveryClient methods.

petrsvihlik commented 4 years ago

Yeah, I agree. I think what you propose is a correct approach. However, I think the Delivery*Response models could use some cleanup in general. We'd like to get rid of the dynamic properties and also of the ContentItem type and simplify the IDeliveryClient interface accordingly. We'll likely address those issues while working on #196 .

If you have any thoughts on the suggested changes, let us please know.

petrsvihlik commented 4 years ago

more info: https://app.intercom.com/a/apps/e42kus8l/inbox/inbox/conversation/26956097388

robertgregorywest commented 4 years ago

I can't access that intercom link I'm afraid - any chance you could paste the details in here?

If you can address ease of testability as part of the clean up on #196 then that would be great. Sounds like quite a large refactor. Getting rid of dynamic would be good - it always feels dirty using it!

Support for Polly also sounds like a good idea, that's a great library.

Any ideas on timings for all this? I have time available so if you want any assistance just let me know.

petrsvihlik commented 4 years ago

@robertgregorywest the intercom link is just an internal ref. it describes basically the same problem. I'm off for a vacation next week and I plan on grooming the stories and coming up with designs in the weeks after I come back. but if you already have something on your mind, feel free to draft a PR and we can work from there.

@Enngage could you please take over this conversation for the time I'm off? thx