Move hysteresis test to gtest #12049
Conversation
|
Playing devils advocate for a minute, don't you think this makes hysteresis slightly easier to misuse? |
Yes, I agree, the API is suboptimal. |
|
@dagar an alternative would be to set a fake clock and inject it into the constructor of the |
|
I don't think shifting some of the complexity to the caller in order to enable faster more convenient unit tests is ultimately going to put us in a better place. Maybe this is a trivial example to get hung up on or maybe that makes it easier to discuss. As much as possible I'd rather we encapsulate these things to make them easy to use, hard to misuse. What if we did something within the framework of lockstep SITL (fake clock), but drove it internally as fast as possible (without the simulator)? |
Yes, I'll think something like that could work. On the other hand many classes - in my opinion - should have less dependencies and side effects, and time is one of those things. |
There was a problem hiding this comment.
I clearly favour this way since:
- The unit test makes sure the logic does what it should without hanging on system calls
- We are not blocked from saving
hrt_absolute_time()calls when we have multiple hysteresis that are checked with the same pace. E.g. it's possible to implemet an efficient hysteresis vector. - It's more modular and hence portable
src/lib/hysteresis/CMakeLists.txt
Outdated
|
|
||
| px4_add_library(hysteresis hysteresis.cpp) | ||
|
|
||
| px4_add_gtest(SRC hysteresis_test.cpp LINKLIBS hysteresis) |
There was a problem hiding this comment.
Could you name the file HysteresisTest.cpp after the convention?
Then the script would automatically handle consistent output: https://github.com/PX4/Firmware/blob/ea48cd4970977f4d189c1a9ba82483ac03852299/cmake/gtest/px4_add_gtest.cmake#L55
This moves the hysteresis test out of the systemlib and makes it its own small library. Since it still depends on hrt_absolute_time this does not link yet. My attempt to get all link dependencies together failed.
|
I changed the filename according to my comment and am rebasing on master since I introduced the conflicts. |
MaEtUgR
force-pushed
the
pr-move-hysteresis-test
branch
from
7765917 to
03e7fb7
Compare
I did the move of the hysteresis class as I envision it. It's now a self-contained simple class without dependencies or side effects and the unit tests run immediately and won't ever by flaky because of stalled CI hosts.
I agree that we still want to run unit tests like these on real hardware, I'm therefore planning to run the gtest tests on a Pixhawk and if that's impossible we need to find some test framework that makes it possible.
I'm not sure yet if every module should come in this form, so without many dependencies, or if we need higher level tests for some "modules".