-
Notifications
You must be signed in to change notification settings - Fork 158
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
Unit tests for ROSPlan (revised) #200
Conversation
Updating forks
… for ProblemGenerator
Merge new commits in KCL-planning to martinkoling fork
…roblem interface working
I am currently trying to find out why in travis CI the tests are not passed but in my computer they do. |
Yes, I and Michael have been wondering why that is the case since I started the project. Travis CI is triggering the test with catkin run_tests command |
…ve .rosinstall file
…est binaries in CMake
Ready for review! |
<arg name="domain_path" default="$(find rosplan_planning_system)/test/turtlebot_demo/domain_turtlebot_demo.pddl" /> | ||
<arg name="problem_path" default="$(find rosplan_planning_system)/test/turtlebot_demo/problem.pddl" /> | ||
<arg name="data_path" default="$(find rosplan_planning_system)/test/turtlebot_demo/" /> | ||
<arg name="domain_path" default="$(find rosplan_planning_system)/test/pddl/turtlebot/domain.pddl" /> |
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.
Does this belong here? not sure if the rosplan_system launches should have hardcoded domains. Same applies to the other launches below.
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.
Short story: yes it does. (see motivation below)
Earlier it pointed to rosplan demos:
<arg name="domain_path" default="$(find rosplan_demos)/common/domain_turtlebot_demo.pddl" />
When rosplan_demos were de attached from main implementation the idea was to make a test domain, which could be reused as well for unit test purposes while keeping this repository self-contained (fully independent of rosplan_demos). Later on, the unit tests grew to include an amazon and a small roomba domain (contributed by @martinkoling), so this change here is an attempt to organize the different domains in folders.
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.
Agree and understood, but still not sure that the planner_interface should have hardcoded paths, the test should, but don't know if the interface one. @m312z what are your thoughts on this? I don't have a clear answer either.
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 unit test launch file does not need a harcoded path, but reuses the example launch file (taking advantage of being a parameterized one).
We could do something like:
<arg name="domain_path" default="foo" />
and tests will still be able to work, however I think is more clear to the user to provide a full self contained example of how to use the launch file.
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.
I agree with Oscar on this one. The launch files contain default values that work (pointing to some example domain) and keep the repo self-contained. I think it is more helpful when learning how things work to see exactly where the domain file is referenced.
Looks good to me now. I can see 41 tests running and passing in the Travis build. From now we should be able to add some more tests as we go. |
Refactored version of #185