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

MAVSDK testing #13075

Closed
wants to merge 46 commits into from
Closed

MAVSDK testing #13075

wants to merge 46 commits into from

Conversation

julianoes
Copy link
Contributor

@julianoes julianoes commented Oct 2, 2019

MOVED TO HERE: #13772

@julianoes julianoes force-pushed the pr-add-mavsdk-tests branch from 6eb3b47 to 4a223b4 Compare October 3, 2019 14:32
mavsdk_tests/.gitignore Outdated Show resolved Hide resolved
@dagar
Copy link
Member

dagar commented Nov 11, 2019

Why catch2 instead of gtest?

@julianoes
Copy link
Contributor Author

Why catch2 instead of gtest?

@dagar:

  • We already have gtest in other places a d I didn't want to bother with the dependencies. catch2 is header only, so it was easy.
  • Now more importantly gtest doesn't use exceptions and therefore a failed assert inside function (not in the top level test) won't cancel the test but is just a macro to return false that you would have to check for. Catch2 uses exceptions so you can assert something in some function and itwill jump out. I wanted to write higher level methods around the MAVSDK APIs, so it was important to have that.

@MaEtUgR
Copy link
Member

MaEtUgR commented Dec 16, 2019

Nice, looking forward to understandable and reproducible integration tests!
Would it be a bad idea to make mavsdk_tests/ another submodule? Or at least sideload catch 2? It seems strange to me to include 17k lines catch 2 into the main repository, the code does not belong to this project...

@julianoes
Copy link
Contributor Author

Or at least sideload catch 2? It

Yes that would be reasonable.

@LorenzMeier
Copy link
Member

@julianoes Could you rebase this PR? I'd like to contribute to it.

This is a workaround to get the landed state published
properly in mavlink_messages.cpp.
This reverts commit 51e1703cbd1bf008bf761ad663147f20d60656f0.
Unfortunately this commit contains two things:
1. Some cleanup and renaiming.
2. An additional wait until lockstep has been initialized.
   By waiting until HIL_SENSOR messages arrive including timestamps we
   stop the startup script and prevent other modules from running until
   time is set up. This should resolve some busy waiting by various
   modules and prevent races on initialization (e.g. the landing state
   being subscribed by mavlink before being published by the land
   detector).
In general, if anything goes wrong in the startup script, we
should fail entirely because things might not work as expected.

In particular, this prevents that we have to press Ctrl+C twice if the
simulator start call is hung waiting for the simulator to appear and
start communicating. We now press Ctrl+C once and exit straightaway
whereas before we would press it once to get the warning:
"Startup script returned with return value: 2",
and then finally exit on the second press.
@julianoes julianoes marked this pull request as ready for review December 17, 2019 09:38
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.

4 participants