[feature request] Make DeviceClient extensible

[tomkerkhove]

Is your feature request related to a problem? Please describe.
Make DeviceClient extensible so that we can build upon it, mock it, etc.

Describe the solution you'd like
I'm happy to open a PR to remove the sealed keyword on the class so that we can extend it.

Describe alternatives you've considered
N/A

Additional context
Relates to #61 & #110 which blocks us from writing tests as well.

Thank you for helping make this project even better!

[tomkerkhove]

Regardless of the language, the described problem is a code smell in your application's design. See chapter 8 of Growing Object-Oriented Software, Guided by Tests for reasons to "Only Mock Types That You Own".

While I get the thought process behind this, if you depend on something as part of your business logic you still want to ensure that it gets called correctly. Today, there is no such way other than having an Azure IoT Hub instance around with pre-configured devices; then we are no longer talking about unit testing.

[fgheysels]

Maybe having an IDeviceClient and IModuleClient interface would be enough. The existing DeviceClient and ModuleClient classes can implement it. People who have those as a dependency can use frameworks like Moq to easily provide a mocked implementation of those interfaces, so that they can test their classes that have a dependency on DeviceClient/ModuleClient.

[tomkerkhove]

That would be fine for me as well, happy to add the interface instead of removing sealed if the maintainers want to.

[vinagesh]

Adding an interface is not going to be feasible in this case. However, we do think that removing the sealed from the clients is an acceptable solution. We will accept your pending PR request.

If this is acceptable, can we close this issue?

[tomkerkhove]

It is for me, I think. Thank you!

Just out of curiosity, why is an interface not possible?

[vinagesh]

Adding an interface means, whenever we add something new, it will break everyone who is implementing the interface. We always try to ship code that is not a breaking change for consumers.

[tomkerkhove]

I think that would be fair but I also see your point.

[drwill-ms]

Specifically our factory methods to create a client would need to change type. We have strict guidelines not to change behavior in the client in ways that would break customers, because every tends to get cranky when their application doesn't build or crashes in new and exciting ways after updating to a new SDK.

FWIW, the SDK is not designed or a built a way we very much like. In many ways we are stuck with decisions from the past. However, we'll be looking at a big client redesign to start fresh to solve a lot of these problems (like unit testable clients, more control over the pipeline, diagnostics, etc.). The Azure SDK team started this effort across all of Azure a few moons ago, and we're getting more and more of our SDKs on board. If you are curious, you can read about the guidelines here: https://azure.github.io/azure-sdk/dotnet_introduction.html.

Thanks for finding a way to make our current client better. We appreciate it. :)

[tomkerkhove]

Thanks for sharing and I'm happy to help!

[vinagesh]

@tomkerkhove - I will be abandoning your PR. Here is the PR which has no build errors - https://github.com/Azure/azure-iot-sdk-csharp/pulls
Once that gets checked in and we have released a new version of the SDK, I will let you know.

[tomkerkhove]

Ok for me!

[tomkerkhove]

Just following up - Is there an ETA for a new version?

[vinagesh]

We just released the new version with these changes - https://github.com/Azure/azure-iot-sdk-csharp/releases/tag/2021-03-18

[ElD4n1]

Hello, thanks for working on this issue, but how is this supposed to be mockable now when the only constructor of ServiceClient is still internal?

Edit: Oh I missed that this request is only about DeviceClient, however, is there any way to make ServiceClient extensible too in the near future?

[vinagesh]

We have made mocking very easy in all the Service Clients if you want to check it out - We have released a version of our clients that is mockable now - https://github.com/Azure/azure-iot-sdk-csharp/releases/tag/preview_2021-6-8

[tomkerkhove]

Awesome, thanks a lot!

[PS99999]

Hello. I am still having problems mocking the DeviceClient class.
I am getting the following error:

Could it be because the only constructor is private, and the existing static methods to create an instance, are using internal components (like the ClientFactory)?

Additionally, if I try to setup method invocation on the mock (e.g.: UpdateReportedPropertiesAsync), I am getting a NotSupported exception saying it is not an overridable member. I guess it would have to be virtual (not sure)?

I am using the 1.37.2 version. I guess it should already support mocking and there is not much difference for the latest).
In terms of Mocking framework I am using Moq.

Does anyone know how to create a mock for this class?
Thank you.

[drwill-ms]

@PS99999 it looks like we added support for mocking the service clients, but not the device client. We'll add it to our backlog and see if we can't get that done. I probably won't be able to promise delivering it anytime soon, likely not before the end of this year, unfortunately.

[PS99999]

Thank you @drwill-ms for looking into it.

[bqstony]

@drwill-ms i have also the problem to mock the ModuleClient class when i testing modulue to module communication. There is the same behavior like in the DeviceClient, the constructurs are internal.

I am using the 1.39.0 version.

Thank you.