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

merging with ApexAI/apex_rostest and repository reorganization #208

Closed
wjwwood opened this issue Mar 20, 2019 · 28 comments

Comments

Projects
None yet
6 participants
@wjwwood
Copy link
Member

commented Mar 20, 2019

See also: ApexAI/apex_rostest#18

We plan to integrate the work done in https://github.com/ApexAI/apex_rostest into this repository and at the same time separate the ROS specific components of launch into a new separate repository (https://github.com/ros2/launch_ros).

This is the planned layout:

  • repository ros2/launch:
    • package launch
    • package launch_testing
      • will be combined with the package apex_launchtest from the repository ApexAI/apex_rostest
      • will also contain a CLI for running tests, name is TBD
    • package launch_testing_ament_cmake (new)
      • will be based on the package apex_launchtest_cmake from the repository ApexAI/apex_rostest
      • will provide cmake integration like add_launch_test(...) (name TDB) which implicitly calls the test runner rather than pytest
        • in the future may optionally invoke pytest instead (still working on integration)
      • will depend on the package launch_testing
  • repository ros2/launch_ros (new)
    • package launch_ros
    • package test_launch_ros
    • package ros2launch
    • package launch_ros_testing (new)
      • will be based on the package apex_launchtest_ros from the repository ApexAI/apex_rostest
      • also, may be based on the launch_testing package in ros2/launch repository as well (depends on what we find)
  • repository ros2/rostest (new)
    • package rostest (new)
      • will contain a placeholder CLI called rostest (or ros2 test?) which will alias to the CLI in launch_testing
        • will allow for future ROS specific options for manual test running
      • may contain cmake macros like add_rostest(...) to allow for future ROS specific integration
      • will serve as the "single point of entry" for ROS 2 users, i.e. they will <test_depend>rostest</test_depend>
      • will contain high level documentation and examples

Action items:

  • Import ApexAI/apex_rostest into ros2/launch @wjwwood
    • Import code to sub folder called apex_rostest
    • Add apex_rostest/AMENT_IGNORE to prevent them from getting picked up by CI
    • open pull request to this repository #209
  • Separate ROS and ROS agnostic code
    • create ros2/launch_ros @wjwwood
    • create first pr on that repository with all history from this repository @wjwwood (ros2/launch_ros#1)
      • delete packages that are not ros specific
    • create pr on this repository removing ros specific packages @wjwwood (#211)
    • update ros2.repos on ros2/ros2 @hidmic (ros2/ros2#676)
    • send announcement to discourse (people will need to update their ros2.repos files) @hidmic
    • migrate issues and pull requests from ros2/launch to ros2/launch_ros as needed @hidmic
  • Integrate imported ApexAI/apex_rostest code @hidmic
  • Create ros2/rostest @hidmic
    • create repository ros2/rostest
    • create rostest package and add to it: (ros2/ros_testing#1)
      • CLI for running tests locally (may be just alias to CLI in launch_testing)
      • documentation for ROS 2 users on how to write and run "ROS tests"

Connected to #161.

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

Some RFC points:

  • Should we have a separate repository for rostest?
    • We could just place this package in the ros2/launch_ros repository.
  • Should we call it something other than rostest (package and/or repository)?
@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Should we call it something other than rostest

I would lean towards yes - in order to be distinguishable / searchable - since it has a completely different API / usage then the package from ROS 1.

@gbiggs

This comment has been minimized.

Copy link

commented Mar 21, 2019

Should we have a separate repository for rostest?

How tightly is it tied to launch?

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

How tightly is it tied to launch?

I don't fully understand what you're asking. It uses launch. I have some bullets above about what would be in it and why it would exist.

@gbiggs

This comment has been minimized.

Copy link

commented Mar 24, 2019

You asked if rostest could just be placed in the launch_ros repository. If it only uses launch, rather than being an integral part of it, then I think it should have its own repository.

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

You asked if rostest could just be placed in the launch_ros repository. If it only uses launch, rather than being an integral part of it, then I think it should have its own repository.

I understand what you mean now, and yes that was our original thinking (it uses launch but it not part of it), but I just wanted to RFC it since it also seemed kind of wasteful since it will end up being (at least for now) very thin itself.

@dejanpan

This comment has been minimized.

Copy link

commented Apr 5, 2019

@pbaughman could you look at the open PRs above? thx

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@hidmic can you collect the pr's that are against various packages to use the new interface under the todo bullet point in the original comment? I know you've connected them to this pr, but it would helpful for people using this issue as an entry point to have them in a bullet list above.

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@wjwwood do you think it's worth to have a separate rostest tool? We can do launch_testalready.

If so, I'm going to need a repo :)

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Does launch_test automatically include the ros specific launch actions?

At the very least we need something like ros2 launch-test? which includes the default ros stuff and then runs the launch file.

I'll create the repository and if we don't need it I'll delete it again.

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Created it https://github.com/ros2/rostest and added your team to it.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Is rostest a good name for this? I can see a lot of confusion between the ROS 1 equivalent and problems with searchability if we use the same name in ROS 2.

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

I asked, there were no alternatives proposed.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Then we should come up with another name - since the concern that rostest is confusing was brought up earlier.

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Does launch_test automatically include the ros specific launch actions?

No, it does not inject them, though I personally think that the tool should not be the one that does it but the user explicitly through some fancy decorator or any of the get_default_launch_description functions. Otherwise you may ran into the situation in which an unaware user duplicates that preamble.

At the very least we need something like ros2 launch-test? which includes the default ros stuff and then runs the launch file.

I see the appeal of a ros2 launch-test CLI but following above rationale I don't see what it would add beyond what launch_test provides already. In my mind, I see launch_test as a pytest replacement if not a plugin (which currently it isn't but it would've been really cool if it was).

If all the above applies, then maybe we don't need the new repo?

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Then we should come up with another name - since the concern that rostest is confusing was brought up earlier.

I'm not going to block the progress on this if no one can suggest an alternative. If you or anyone else has a better idea (I don't) then please suggest it, otherwise we need to continue.


No, it does not inject them, though I personally think that the tool should not be the one that does it but the user explicitly through some fancy decorator or any of the get_default_launch_description functions. Otherwise you may ran into the situation in which an unaware user duplicates that preamble.

If every test has the decorator, then I don't agree. It would be better to include it automatically, otherwise it becomes just boilerplate. Why would the user include that themselves? I saw that happened on the composition launch file pr, but I don't know what caused that confusion.

I can see, somewhat, the point of view that for testing you might have the user include the default launch description manually on each of the tests, since they're using the launch API directly rather than a tool, but for plain ros2 launch, I don't agree that it shouldn't be injecting anything.

I see the appeal of a ros2 launch-test CLI but following above rationale I don't see what it would add beyond what launch_test provides already. In my mind, I see launch_test as a pytest replacement if not a plugin (which currently it isn't but it would've been really cool if it was).

It may be an almost empty script or one that just invokes pytest with or without a plugin or something for now, but in the future it is needed to support XML based test files. You can't just invoke pytest on one of those, and requiring the user to write a pytest or python script just to invoke a launch XML where one of the processes is a gtest seems like an unnecessary burden.

Also, please consider my original summary for the purpose of the rostest package:

  • repository ros2/rostest (new)
    • package rostest (new)
      • will contain a placeholder CLI called rostest (or ros2 test?) which will alias to the CLI in launch_testing
        • will allow for future ROS specific options for manual test running
      • may contain cmake macros like add_rostest(...) to allow for future ROS specific integration
      • will serve as the "single point of entry" for ROS 2 users, i.e. they will <test_depend>rostest</test_depend>
      • will contain high level documentation and examples

It goes beyond the command line tool.

@pbaughman

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Would it solve the problem is there was a ros2 test verb package that include launch_testing, launch_testing_ros, and the package with the cmake functions? That would be one single point of entry for testing.

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

That package would be called ros2test, which is ok by me. The only hesitation I have is that ros2X usually (until now) only included the command line tool and was pure python, this one would need to potentially contain more and be a CMake package instead.

@dejanpan

This comment has been minimized.

Copy link

commented Apr 28, 2019

I second ros2test name.

@hidmic what is holding back above PRs to be merged? we looked them up yesterday and they were all approved.

Do you need any help?

@hidmic

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

@dejanpan after addressing all review comments, I started to see test failures locally. I'm still solving/fighting them. I plan to get this all in by Monday EOD, hopefully.

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

@hidmic I tentatively moved this into "done" for the Dashing API freeze, feel free to disagree.

@hidmic

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@wjwwood may I ask you to create the rostest repository? Nevermind, saw above's comment.

@hidmic

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@wjwwood I just realized I never posted my reply to #208 (comment).

If every test has the decorator, then I don't agree. It would be better to include it automatically, otherwise it becomes just boilerplate. Why would the user include that themselves? I saw that happened on the composition launch file pr, but I don't know what caused that confusion.

I generally agree with the idea of reducing boilerplate as much as possible, and thus maybe a decorator is not the best solution. But regardless of which mechanism we may chose, I do think that explicit is almost always better than implicit.

This is a special case, though, because every test would need it, just like every ROS aware launch file does. Injecting a launch description preamble into launch tests fixtures as they are today is somewhat tricky though, and requires some amount of plumbing, specially considering that the base launch test CLI has to remain ROS agnostic.

So I started to wonder why we even need this in the first place. There's a large coupling between launch_ros entities and the preamble, some actions implementations rely on the preamble to have been executed before and fail ungracefully if it hasn't. Wouldn't it be better for actions to take care of context initialization themselves? On execution, every action can check if the context has been initialized for a ROS aware launch and lazy initialize it if not. Concurrency issues are not a problem (i.e. launch runs on main thread asyncio loops only), but even if they were we can always lock the context. And this way we remove the need for preambles altogether. I'd like to hear the reasons behind the preamble though.

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Basically the idea was that the preamble remain optional and replaceable, if I initialized them via the other actions then these two characteristics are hard to ensure. I guess you could have a function that replaces the default before you launch (and you could replace with empty). I'm not too worried about this point, but it was something that I was thinking about.

Also, I very much wanted to dog-food this feature, by using existing features in launch, so adding some default actions in the command line tool was the least invasive option. I don't know how we would implement what you're describing, but my first thought would be an action that "ensures" the services exist, starting them if needed and doing nothing if they are already there. But that means when introspecting you'll see one of these actions with every lifecycle node or other feature that needs the rclpy node to work properly, and I think that would get noisy when introspecting. So then we either need to do something more sneaky, which I want to avoid and instead dog-food existing API's, or implement a new feature to "hide" actions that are considered details or something like that, and I fear that will be abused.

All that being said, I think having the actions that need the features ensure they are started (whether that be with another action or something else) is not a bad idea.

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Also, to extend my first point about making this infrastructure replaceable, for testing a Node might need us to start a rclpy node when testing but not when launching, whereas a lifecycle node might require that when testing and launching, so it might not be straightforward to add an action to each other action which "needs" an rclpy node, because it may not be clear when that is. That's another benefit to doing it when you launch the launch file, because you can condition it based on the context you're running in rather than the content of the launch file.

@hidmic

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Hmm, I see the benefit for testing now. And yeah, we should add a check for the features to be present, though if we keep the preamble that can wait.

Alright, fair enough, I'll take care of ros2test except for the preamble injection and figure out how to do the plumbing afterwards. cc @pbaughman in case has some ideas on this regard.

@pbaughman

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

May or may not be helpful, but I just add the ROS specific startup stuff to the test launch descriptions that need it, and don't add it to the ones that don't. It's not elegant and it requires you to understand what's going on behind the scenes a little, but it works for me.

@wjwwood

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

it requires you to understand what's going on behind the scenes a little

Yeah, I'm afraid that's not going to work at scale, plus as I alluded to above, what "needs it under the hood" may change over time or based on the scenario, so it's probably best something the user doesn't have to declare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.