Functionality of Stream class should not be mocked by default

[lasselukkari]

First of all, thanks for putting together this amazing project! It has been really useful for me.

Here is my situation. I have a class that inherits the Stream by wrapping another instance of a Stream. I do this so that my code works with any Stream input like Wifi, Ethernet and Serial without any changes. It also makes it compatible with anything else that expects a Stream like JSON parsers and so on. The Stream base class should the same on every Arduino. This is the key feature to write cross platform libraries for all Arduinos.

Because this project implements a non standard Stream where some of the expected functions are missing or mocked with the "GODMODE" testing anything that uses the Stream is impossible. For example, any class that is derived from the Stream should be able to access the protected Stream function timedRead or set the _timeout property.

I believe the Stream class should be left as it is with the real platforms. To be honest, in my opinion all the mocks for classes that are derived from Print, Stream or Client are rather pointless. Instead people should write their code to work with the base classes and for testing a mocked class that implements the virtual functions should be used instead. This is the standard practice to write testable code but now it is impossible. I feel that this decision actually encourages bad programming practises. Maybe providing mock for the HardwareSerial is useful because that is normally used directly but atleast the Stream should stay as it is.

I had to fork this project to be able to move forward. I'm fine with that. I just feel that this may cause problems for many others too and may be the wrong approach.

What are your thoughts on this? Have a nice weekend!

[ianfixes]

Thanks for writing!

The main thing I'm hearing in what you wrote is:

Bug: protected Stream functions like timedRead and/or the _timeout property are not defined

I'm changing the issue title to this, and I'd welcome any info you have on how you'd expect these functions to work during testing (as I don't have much personal experience with the deeper parts of Stream).

As far as the need for Stream (as well as all of the mocked classes I provide in this project) is that Arduino's libraries were written for an AVR backend and simply will not compile at all on any other architecture.

As far as testing, you're right -- in a perfect world people would use dependency injection and isolate themselves from low-level components. But the Arduino ecosystem was built without testing in mind, so between the hard-coded assumption that you will have a setup() and loop() entry point and the fact that you can only compile on AVR architecture really limited my options.

Even the idea of mocking the low-level boards support (i.e. your target architecture is a software emulator) is problematic, because then you have to write one mock per board type.

I still hope against hope that the Arduino folks will push for more than compilation tests. The Adafruit team seems to be driving that effort and "unit tests" don't seem to be in their vocabulary. I've mentioned unit testing a few times on the developer mailing list and get absolute silence in response.

[lasselukkari]

The Stream works just fine if you just copy the existing original Stream.h and Stream.cpp from the Arduino core. This is what I did in my own fork. There should be no need to modify the Stream at all and it just causes problems. If you really need to mock for example the HardwareSerial do it just there and leave the Stream as it is.

The non virtual functions in the Stream only use the virtual functions that are implemented in the derived classes. There is not much depencies to other Arduino code there.

The project that I'm testing using this is here: https://github.com/lasselukkari/aWOT

[ianfixes]

I received some helpful information that might be relevant to this issue. At the very least, I can see that my reverse-engineering of the IDE's g++ invocation wasn't totally accurate. (Compare the listing in that issue comment with the flags I use, -std= and -O1 in particular.)

I think the proper thing for me to do here is take a step back and try to rebuild the mocks based on the _ correct_ set of compiler flags. @lasselukkari is that something you'd be interested in being involved with?

[matthijskooijman]

I believe that the proper approach here would be to keep Stream completely unmodified compared to the Arduino version, since it's just an abstract superclass that can compile on a PC just fine. Then any mocking should be done in subclasses. Something like a MockedStream class that extends Stream and is again extended by HardwareSerial would make sense. It seems there is already TapedStream class that occupies this place, but instead of being implemented on top of the standard Arduino Stream class (by just implementing the read and write methods), it seems to be heavily coupled to changes in the Stream class.

Also see #145 for some thoughts on further decoupling the mocking behaviour form the object being mocked.

[ianfixes]

I've opened #146 ... I should have opened it sooner.

[lasselukkari]

The Arduino project has moved the common pieces to a shared repo: https://github.com/arduino/ArduinoCore-API. IMO these files should be taken there and left as unmodified as possible.

I see that you are working hard currently to add new features. Thanks for that. But I would like to see more simplification and clean up than features. I would like to contribute to the project but unfortunately I do not agree with the general direction at the moment. I think there should be a more clear separation with the Arduino core and the testing related functionality.

[ianfixes]

It's funny that you bring this up, because I've been looking at revisiting the entire set of C++ mocks. The feeling of "overcomplicating it" was exactly what I thought of when looking at an "official" Arduino.h.

In the beginning I was just creating compilation mocks as fast as I could and brute-forcing my way through compiler errors. Things like the hardware need to be mocked, but everything else can/should just use the existing libraries as much as possible! I think the only snag might be pgmspace.h and how it interacts with String and whatnot. I can't remember exactly how that shook out.

Big picture is that I need to create viable mocks for each platform. For example, what avr-libc provides, which is the general topic of #241 and #251

I do not agree with the general direction at the moment

What do you perceive that direction to be?