Open-Smartwatch / open-smartwatch-os

The Open-Smartwatch Operating System.
https://open-smartwatch.github.io
GNU General Public License v3.0
998 stars 163 forks source link

Create the notification system #337

Closed xbohdan closed 1 year ago

simonmicro commented 1 year ago

Ohmmm, why did you delete all unit-tests except yours?

akmal-threepointsix commented 1 year ago

Ohmmm, why did you delete all unit-tests except yours?

Hello, I am the second contributor of this PR.

Were these unit tests necessary? I deleted them because:

simonmicro commented 1 year ago
* `logging.cpp` didn't compare obtained and expected results (no calls to `EXPECT` or `ASSERT`), so it didn't automatically test anything, just printed some text to console and litteted the actual tests outputs.

Yes, it just prints to the console, as I had never any motivation to capture the output and check it - so, yes it is not really testing... Except the fact that it test compile-ability (take a look at the chained templates inside the OswLogger-class), running without crashing and (if I look at them) "do they look right". Feel free to friend / inherit the OswLogger class and patch in the output redirect necessary, or open an issue - maybe I'll fix that... -> nvm, I did it myself #338

* `string.cpp` I deleted them because I was confused by `String` class which inherits from `std:string` (and it's not a good practice to inherit from std classes). But now, as I can see, we need this `String` class for Arduino, am I right? In this case we should return these tests, they seem good and useful.

Indeed - furthermore, the String class has a lot of custom (hacky) features to work like the Arduino ones and I broke it more than once by accident. Therefore there are tests - and while inheriting from std::string is not so cool, it allows the use of other libraries out there, which are not written for Arduino - take "ArduinoJson", which on the PC-platform only compiles with std::string support. Finally, I saved me a lot of work to re-implement another custom String-class by just reusing std::string :)

simonmicro commented 1 year ago

Regarding the missing unit-test checks for the logger: There you go (I've just had some spare time) -> https://github.com/Open-Smartwatch/open-smartwatch-os/pull/341