-
Notifications
You must be signed in to change notification settings - Fork 59
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
Feature/integration test #55
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: youliang <[email protected]>
Signed-off-by: youliang <[email protected]>
Signed-off-by: youliang <[email protected]>
84258d8
to
f1ba9b9
Compare
Signed-off-by: youliang <[email protected]>
Signed-off-by: youliang <[email protected]>
hihi! Sorry It took so long to get back to you. I have actually been working on a similar project to do testing, and hopefully we can work out how to combine the efforts. Or perhaps we can have both running as well. This PR seems to be a superset of what this PR does and we could combine the two: #53, but we should check in with @ddengster ? I might be misunderstanding something. |
PR #53 builds packages from the source repos and does the unit tests for each package. It seems that this PR uses a docker image, would that have any difference? Would be happy to scrap it if there isnt |
think the docker image comes from here: https://github.com/open-rmf/rmf_demos/blob/main/.github/workflows/build.yaml which is rebuilt every time a push to main is done. 🤔 |
I think both build approaches provide different values. #53 is a cleaner approach as it uses git This integration test focuses on running |
@ddengster any thoughts? If its ok, we could merge this PR in and scrap the ros-tooling setup ( at expense to your efforts 🙇 ) i |
My other concern is whether we should put this integration test elsewhere; the intention of this repo seems to be a link to other rmf repos. I wonder if a dedicated org-wide ci repo is worth setting up just for that. Thoughts @mxgrey? Otherwise we can merge this. As for #53 it's blocked by other issues. I'll attend to it later. |
I think having all the different tests is a good thing, since they're testing different processes. #53 is important for making sure the entire stack of This PR is good for making sure that demos work and that new users have a predictable experience with the demos. That being said, since this PR depends on build artifacts from the |
I'd be open to this too. I guess it's a question of how we want to expose the integration tests to casual users. Should the integration tests be plainly available from this |
from a visibility point of view, I think a dedicated ci repo would be helpful 👍 |
Well, I think we are going in the direction of using both tests, so since the other one is pending I'll go ahead and approve this for merging. Sorry it took so long! |
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.
The docker build might break due to the gpg issue, but looks great! Sorry it took so long.
This introduces a CI with an e2e rmf integration test (which can also run nightly).
The Integration test,
tests/integration_test.py
will launch 3 rmf scenarios (office, clinic, airport), and dispatch ofLoop
,Delivery
, andClean
tasks. The script will make sure all tasks are completed successfully before the specified timeout, else raise an error.To try the script locally:
On some rare cases, the tasks will fail due to some stray reasons (coke drops during delivery, caddy not launch successfully, robot stuck at a corridor as path is too narrow...). All these will be addressed separately.
Feedbacks are welcome to identify the best method to implement this