Runtime: Not able to mock SerialPort

[kpakur]

Hi,
dotnet core 3.1, System.IO.Ports 4.7.0.

Methods and properties in the SerialPort class are not virtual and therefore it is not possible to mock them with tools like Moq.
Similar issue with SerialDataReceivedEventArgs is not possible to use in Moq Raise.

Please make the SerialPort class methods and properties virtual.
src/libraries/System.IO.Ports/src/System/IO/Ports/SerialPort.cs
and also SerialDataReceivedEventArgs constructor public (instead of internal)

Note: Arguing that it should not be mocked and other techniques should be used is not intended to be part of this discussion. There are different approaches and opinions about it and that's fine as long as all options are possible.

Thanks,
Kamil

[patrickklaeren]

Put a facade around SerialPort and achieve your specific usecase. @kpakur

[kpakur]

@Inzanit That's what I did eventually (3 facades needed btw for SerialPort, SerialDataReceivedEventHandler, SerialDataReceivedEventArgs to be able to handle DataRecivedevent). Now I have 3 additional classes that actually do nothing but just allow unit testing of my custom logic around data received from the serial port.

I consider this as a waste of time.

Again I want to point out that it was not my intention to start a topic of how to overcome this, or a "do not mock a type you do not own" type of discussion but rather to improve and make the life easier for anyone dealing with that implementation.

[john-h-k]

I consider this as a waste of time.

Why will they hurt performance and change design by making stuff abstract when there is a perfectly good and relatively simple workaround?

[kpakur]

@john-h-k I have one class that has my bespoke logic to handle data coming in from the SerialPort and I wanted to test that logic. In order to test my class I wanted to isolate my class from the hardware. Injecting mock of SerialPort into my class, pretending it sends some data and assert my logic works as expected.

However it was not possible to do such mock. So I ended up writing 3 wrapper classes that do nothing more than make all properties, methods and events virtual for SerialPort class. And it was a waste of time because if the members of SerialPort were virtual already I would not need to write that workaround. Especially that writing these wrapper classes took me more than actually writing my bespoke logic that I wanted to test in the first instance.

So if we take a wider view, and actually look at my little IoT Edge C# module that has 2 classes Program.cs and MySimpleLogic.cs plus 4+ wrapper classes just to make it possible to inject mocks to test my code then it does look a bit overwritten to be honest. (yes 4+ because the IoT Edge is using ModuleClient to handle events which is a sealed class... so I needed to wrap it too).

Yes they are relatively simple workarounds for someone who likes WET principle (WET - we enjoy typing).

[john-h-k]

Yes they are relatively simple workarounds for someone who likes WET principle (WET - we enjoy typing).

Is this RANAODIJMYWUCGYHTEIIAA? (RANAODIJMYWUCGYHTEIIAA- really a necessary abbreviation, or does it just make your writing unnecessarily confusing, given you have to explain it immediately afterwards anyway). 😄

Making methods virtual can have a non trivial performance effect. It seems a non-starter to regress performance for this purpose when there is a workaround, even if it isn't that nice. However it depends on the general overhead of SerialPort, which I can't speak for, to whether making them virtual is an issue

[kpakur]

Making methods virtual can have a non trivial performance effect.

@john-h-k please provide a _trusted_ and up to date source of that information.

[john-h-k]

@kpakur literally just look at the asm for a virt/non virt call. When it can't be devirtualised it is an indirect call. It also prevents inlining https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBgDVeUDB2ySaAbxoNbDANoBZGBgAWEACYBJcZIAUzm6ePmKSAPJifBBcuCwAchBeXJK8XKkA5gCUALo2djoMSiZmkgypGAwAYhAQfpkMlkwA7Ax0agwAvjRd1Fq6ZAwJXEbF5lZ5tk4u7t6+AdPBvhFRMfGJyakZORNMuuVVNXUNza3tPT19egxCDTsF+yMYfo8lhfXW1HZfJwos1bX1ABUDFIGk+dh6Xzuex4gwgGEefiGL3Mb1u4O+Pz+hyBILBX3OQA==

[kpakur]

@john-h-k Indeed there is an additional step, however you mentioned it is non-trivial performance effect when I think actually it does not make a huge difference with the hardware we have nowadays. So are there any up to date benchmarks so we can compare how much does it matter? Especially that we talk here about SerialPort...

[nathanpovo]

I agree that with @kpakur that it is tedious to create additional classes just to be able to mock the SerialPort class.

Perhaps it would make more sense to create an interface and implement that interface in the SerialPort class rather than converting it to an abstract class? Would that reduce the possible performance penalty mentioned by @john-h-k ?

Having an interface of the SerialPort class would make consumers of the interface much more easily testable without having to go through the work of creating additional classes/interfaces or without having to use third-party libraries (such as https://github.com/jcurl/SerialPortStream).