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

add unit tests #14

Merged
merged 1 commit into from
Jun 23, 2017
Merged

add unit tests #14

merged 1 commit into from
Jun 23, 2017

Conversation

dirk-thomas
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Ready for review.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Jun 22, 2017
@dirk-thomas dirk-thomas self-assigned this Jun 22, 2017
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm


count = 0
for package_name in package_names:
package_cmd = ['ros2', 'msg', 'package', package_name]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice this verb before. It looks strange to me, since package is not a verb in this context. I would have expected something more like ros2 msg list --package <pkg> or ros2 msg list --only-package <pkg>. I know that's not part of this pr, but I think it probably should be changed in a follow up pr.

Related to this is the ros2 msg packages command, which reads ok, but again is not a verb. I think ros2 msg list_packages or ros2 msg list-packages or ros2 msg list --packages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same applies to ros2 srv package[s] verbs.

@ros2/team can you guys weigh in on this? Do you prefer ros2 msg package <pkg> or ros2 msg list --package <pkg> (also ros2 msg packages vs. ros2 msg list --packages)?

If the consensus is that the alternative verb names I proposed would be better, then I'll open a pr to update the commands later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage I saw with having separate "verbs" is that each of them is self contained. Additional ones can easily be added (without fiddling with existing code, e.g. by external Python packages). Also with a single "verb" handling multiple different cases you have to "read" the usage to figure out which option can be used together with others and how it might affect the output (e.g. the --packages and --package options would be mutually exclusive).

@dirk-thomas
Copy link
Member Author

I will go ahead and merge these tests. Please continue the conversation / reply to @wjwwood question.

@dirk-thomas dirk-thomas merged commit 5fe3be6 into master Jun 23, 2017
@dirk-thomas dirk-thomas deleted the add_tests branch June 23, 2017 23:01
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Jun 23, 2017
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
* Give more info when output is not in expected sequence
* Real captured output has newline in it.  Update test data to reflect
* Fix exception with process name resolution, add test
* Add infrastructure to coordinate ROS_DOMAIN_IDs between apex_launchtest processes
* Isolate tests with ROS_DOMAIN_ID by default
* Address PR comments
* Fix style
  - Defaults in the docker image we run the test in changed. We're now failing for style
    reasons. Mostly need to re-order imports and change " quotes to '
* apex_launchtest_ros add better example
  - Fix up style issues
  - Typo
  - Update apex_launchtest_ros/examples/README.md
  - Formatting
  - formatting
  - Fix bad spelling
  - Improve sentence
  - Fix Typo
* Improve apex launchtest ros
  - Add the parametrization infrastrcture
  - Refactor the apex runner into an ApexRunner and a RunnerWorker
  - This veresion should still run existing tests.  This is mostly a refactor commit
* Refactor to allow for parametrized launch descriptions
  - Combine pre and post results into one set of results, add class names so the tests
    are distinguishable
  - Update tests to match new 'run' API that returns a dictionary of {test_run: result} instead
    of a tuple of (pre_shutdown, post_shutdown) results
* Add parametrized example, finish refactor
* Beef up test coverage for new features
* Push changes into apex_rostest subdirectory.
* Prune unnecesary files.

Co-Authored-By: pbaughman <dblue135@yahoo.com>
Signed-off-by: Pete Baughman <pete.baughman@apex.ai>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
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.

2 participants