search

opendroneid / opendroneid-core-c

Open Drone ID Core C Library
Apache License 2.0
170 stars 64 forks source link

Beacon unit test #86

Closed StephanePiskorskiSF closed 8 months ago

StephanePiskorskiSF commented 8 months ago

Add a unit test on the beacon frame encoding, runnable from a CI to check against regressions.

friissoren commented 8 months ago

How exactly are you supposed to compile and run this new test? It would be great if you could add some documentation related to this.

I tried cmake .. in the test/build directory and while that works, it spits out a lot of warnings (I am on Ubuntu 22.04 with cmake 3.22.1). Probably some of those could be easily removed.

However, when then doing make afterwards, it fails with

[100%] Linking CXX executable unit_odid_wifi_beacon
/usr/bin/ld: cannot find -lopendroneid: No such file or directory
/usr/bin/ld: cannot find -lThreads::Threads: No such file or directory
StephanePiskorskiSF commented 8 months ago

Hi Søren,

Thank you for your review. It is my first time trying GitHub and I forgot to add a description or even just say hello and introduce myself, sorry about that.

Compiling and running the test has to be done just like it is done in the GitHub workflow. i.e. the test is not a standalone CMake project, but was just inserted in the same place as the already existing test, except it is marked as being an actual CTest instead of a simple executable. Therefore you need to build the root project in order to build the new test along.

The GitHub workflow seems to have worked fine since I can see the test result in https://github.com/opendroneid/opendroneid-core-c/actions/runs/7459545630/job/20299910103?pr=86

I will add a few words in the How to Build section of the README file to talk about the test. It might be also a good thing to separate tests into different folders, but we need to agree on a directory structure. I will try to improve on that tomorrow.

friissoren commented 8 months ago

Ahh, right. I missed the addition to the top-level CMakeLists.txt.

I will wait for your additions and then this can be merged.

friissoren commented 8 months ago

Thanks. This is a valuable contribution. Of course, the test set is rather limited right now, but since the general framework is there, it should be fairly easy to add additional tests.

StephanePiskorskiSF commented 8 months ago

Thanks. This is a valuable contribution. Of course, the test set is rather limited right now, but since the general framework is there, it should be fairly easy to add additional tests.

You're welcome. Hopefully I will have some spare time to add a few more tests. My goal is to keep our internal copy at senseFly and the GitHub reference not too far from each other.