Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing/integration tests #367

Merged

Conversation

akmal-threepointsix
Copy link
Contributor

@akmal-threepointsix akmal-threepointsix commented Jul 9, 2023

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:

  • Added ImGUI Test Engine submodule (CMakeLists.txt, emulator/lib/ImGUI_TestEngine). IMPORTANT: since I added new submodule, you need to run $ git submodule update --init --recursive after checkout to this branch
  • Updated ImGUI, because older version was incompatible with ImGUI Test Engine (emulator/lib/ImGUI)
  • Added UI tests for Alarm and Timer (emulator/src/UiTests/OswAppAlarm.cpp, emulator/src/UiTests/OswAppTimer.cpp)
  • Updated README.md with new testing chapter
  • I didn't change any production code, because testing must not interfere with business logic. I only added three friend classes in three header files (emulator/include/Emulator.hpp, include/apps/OswAppDrawer.h and include/apps/OswAppV2Compat.h)

And some NEW changes:

  • I fixed all errors related to the newest PR #358. It made my UI testing code a bit uglier (e.g. I had to use a lot of casting), but at the time I am writing this, everything at least works...
  • In UiTest_main.hpp, I added some code to skip tutorial, if emulator was launched with --ui_tests option

Fix errors in Emulator.cpp, because some files in ImGUI were renamed from "sdl" to "sdl2"
Update is necessary for compatibility with ImGUI Test Engine
Because I plan to add UI Tests
Add UI tests for alarm and timer
Fix all errors related to merge
@simonmicro simonmicro self-requested a review July 13, 2023 20:50
@simonmicro simonmicro self-assigned this Jul 13, 2023
@simonmicro simonmicro added ⭐ enhancement New feature or request src/core 📜 documentation This adds / changes the documentation src/osw-emulator labels Jul 13, 2023
@simonmicro
Copy link
Member

simonmicro commented Jul 13, 2023

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 😉

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
Copy link
Contributor Author

akmal-threepointsix commented Jul 14, 2023

  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
Copy link
Contributor Author

akmal-threepointsix commented Jul 15, 2023

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
Copy link
Member

@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 😉

@akmal-threepointsix
Copy link
Contributor Author

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
Copy link
Contributor Author

@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

@simonmicro simonmicro merged commit f2746e7 into Open-Smartwatch:develop Jul 30, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 documentation This adds / changes the documentation src/core src/osw-emulator ⭐ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants