Open-Smartwatch / open-smartwatch-os

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

Testing/integration tests #367

Closed akmal-threepointsix closed 11 months ago

akmal-threepointsix commented 11 months ago

Since this old PR #351 diverged too much from the up-to-date develop branch, I decided to create a new branch from the develop and open this new PR.

These changes are basically the same as in PR #351:

And some NEW changes:

simonmicro commented 11 months ago

Hi, thanks for your work - again! So two things I've noted here...

  1. Meh - le pipeline is failing...
  2. Could you reorder the code to bundle all the unit/ui/helpers stuff for testing under src/tests again? You've moved them out and put them into the "regular" code. Do not worry, if you won't do it I'll clean that up :wink:

After you put so much work into it to re-submit this PR I'll absolutely make sure to get it merged this time. This is the next thing going in - so I'll not allow this to run out-of-sync again! Hehe...

P.S. There is so much code, I'll try to dive deeper into it at a later date - for now I also have too much of my own work going on...

akmal-threepointsix commented 11 months ago
  1. Meh - le pipeline is failing...

Fixed (hopefully) by pushing ImGui Test Engine submodule folder

  1. Could you reorder the code to bundle all the unit/ui/helpers stuff for testing under src/tests again? You've moved them out and put them into the "regular" code. Do not worry, if you won't do it I'll clean that up πŸ˜‰

Done. I forgot about this one :)

p.s: in my last PR it wasn't your fault, we were playing with rebases and the situation got out of hand :) Old commits were rebased and git thought they were new commits, because their hash was different. At least I think that's what happened... Anyway, it was too painful to fix...

akmal-threepointsix commented 11 months ago

Ok, I also removed reinterpret_cast from tests/helpers/TestDrawer.h.

It turned out, that OswAppV1 and OswApp are the same class, so I can simply static_cast timer and alarm from OswAppV1 to OswAppTimer and OswAppAlarm respectively, because they are inherited from OswApp

simonmicro commented 11 months ago

@akmal-threepointsix If you formulate that like this... Ever thought about the more safe variant of https://en.cppreference.com/w/cpp/language/dynamic_cast ? This also ensures you won't cast in the future if we break it :wink:

akmal-threepointsix commented 11 months ago

Yes, sure, dynamic_cast is also a valid option and safer one. However, it introduces some run-time overhead, which in this case seemed unnecessary to me. On the other hand, you are right, in future someone might change codebase and static_cast will silently fail, so it would be better to use dynamic_cast. I believe, run-time cost is insignificant, especially since it is in emulator & UI tests environment, not in production.

akmal-threepointsix commented 11 months ago

@akmal-threepointsix If you formulate that like this... Ever thought about the more safe variant of https://en.cppreference.com/w/cpp/language/dynamic_cast ? This also ensures you won't cast in the future if we break it πŸ˜‰

Ok, done. Changed static_cast to dynamic_cast and added assert to confirm that casting was successful and didn't return nullptr