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

Beacon unit test #86

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

StephanePiskorskiSF
Copy link
Contributor

@StephanePiskorskiSF StephanePiskorskiSF commented Jan 9, 2024

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

@StephanePiskorskiSF StephanePiskorskiSF marked this pull request as draft January 9, 2024 09:31
@StephanePiskorskiSF StephanePiskorskiSF marked this pull request as ready for review January 9, 2024 10:08
@friissoren
Copy link
Contributor

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

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

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 friissoren merged commit 2a239f1 into opendroneid:master Jan 10, 2024
1 check passed
@friissoren
Copy link
Contributor

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants