-
Notifications
You must be signed in to change notification settings - Fork 13
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
Mission tests: kill LRAUV application after enough iterations #122
Conversation
Signed-off-by: Louise Poubel <louise@openrobotics.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failing on LrauvTestFixture.Command
?
The 4 mission tests in this PR pass for me locally. (Tested in Garden because I've deleted my Fortress workspace 😅)
Locally, the terminal does not return cleanly for you too, right? I have to type reset
, otherwise what I type doesn't show up. I feel like we've had this conversation before. Maybe I'll add it to #4 so I don't have to ask this again.
(I forgot to comment out the debug lines for the barycentric interpolation, so CI is now printing a lot of junk. Will open small PR) |
Looks like |
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Ouch, two tests had the same name. Fixed name in e0aba90, and also tried to make the test more robust |
|
Signed-off-by: Louise Poubel <louise@openrobotics.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI passing, looks good.
😅 Ha okay so we did have that conversation before.
Thanks @chapulina! I think this will work for now, but in general, terminating the app while it's running (without properly uninitializing) might cause some issues. Particularly, on the next startup the vehicle will throw a critical failure stating that the last restart was unintentional - this mechanism was put out in place to notify the operator in case the application crashes while the vehicle is deployed at sea (the watchdog timer automatically reboots the system if the app crashes and the app is launched at startup). After an unintentional boot the app will ignore any persisted configuration settings that are found in The way this works is that the app creates an empty file in the Data folder called I don't think this matters for CI testing, but wanted to make you aware of this. Once we resolve #4 we should adjust mission timeouts to work with CI expectations. Thanks! |
Thanks for all the context, @braanan . For automated tests, I think ultimately it would be ideal if we could start the controller via code, as opposed to launching the LRAUV CLI. This way we wouldn't need to fork a process, which brings complications like #39. I don't know how feasible or desirable it would be to expose a public API for the controller, but I thought I should mention it anyway. |
Thanks to @braanan , now the missions don't end up in critical errors anymore. Which means that they run until their timeouts, which are very large (hours).
Our tests had been relying on the missions ending relatively quickly until now, and with the latest fixes, they're running forever (I had to cancel GitHub actions that was running for 1.5 hours on #121).
Instead of updating the mission timeouts and relying on the LRAUV application terminating on its own, the idea on this PR is that the tests should know when they've received enough data and terminate the application then.
We've been killing the application anyway because of #4 , so now we're actually taking advantage of that.