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

Mission tests: kill LRAUV application after enough iterations #122

Merged
merged 4 commits into from
Dec 18, 2021

Conversation

chapulina
Copy link
Contributor

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.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Collaborator

@mabelzhang mabelzhang left a 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.

lrauv_ignition_plugins/test/test_mission_depth_vbs.cc Outdated Show resolved Hide resolved
lrauv_ignition_plugins/test/test_mission_depth_vbs.cc Outdated Show resolved Hide resolved
@mabelzhang
Copy link
Collaborator

mabelzhang commented Dec 18, 2021

(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)
Update: opened #123. CI should be easier to read without those debug lines.

@mabelzhang
Copy link
Collaborator

Looks like LrauvTestFixture.Command is run twice, test 2 passed, and test 13 failed.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

Looks like LrauvTestFixture.Command is run twice, test 2 passed, and test 13 failed.

Ouch, two tests had the same name. Fixed name in e0aba90, and also tried to make the test more robust

@chapulina
Copy link
Contributor Author

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.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Collaborator

@mabelzhang mabelzhang left a 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.

@chapulina chapulina merged commit dc6e5ce into main Dec 18, 2021
@chapulina chapulina deleted the chapulina/killLRAUV branch December 18, 2021 04:19
@braanan
Copy link
Collaborator

braanan commented Dec 18, 2021

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 Data/persisted.cfg - this happens so the operator gets an opportunity to reconfigure the vehicle if a config that was added at run time is causing issues.

The way this works is that the app creates an empty file in the Data folder called running, which is removed as part of the normal teardown. That way if Data/running is present at startup the app can deduce that an unintentional reboot happened.

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!

@chapulina
Copy link
Contributor Author

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.

@chapulina chapulina mentioned this pull request Nov 17, 2021
5 tasks
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.

3 participants