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

Merge apex_launchtest functionality into launch_testing #215

Merged
merged 28 commits into from
Apr 30, 2019

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Mar 22, 2019

This pull request merges functionality from apex_launchtest and apex_launchtest_cmake packages into launch_testing and launch_testing_ament_cmake respectively. Some renaming and style corrections were applied as well.

Additionally, a new package test_launch_testing is introduced to avoid launch_testing from being an ament_cmake type of package.

Connected to #208.

@hidmic hidmic requested a review from wjwwood March 22, 2019 18:12
@hidmic hidmic added the in progress Actively being worked on (Kanban column) label Mar 22, 2019
@hidmic
Copy link
Contributor Author

hidmic commented Mar 22, 2019

CI (full run):

  • Linux Build Status (unrelated rosidl_generator_c linter errors)
  • Linux-aarch64 Build Status (unrelated rosidl_generator_c linter errors)
  • Packaging Linux Build Status
  • macOS Build Status (unrelated rosidl_generator_c linter errors)
  • Windows Build Status (unrelated rosidl_generator_c linter errors)

@hidmic hidmic added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 22, 2019
launch_testing/README.md Outdated Show resolved Hide resolved
test_launch_testing/package.xml Outdated Show resolved Hide resolved
test_launch_testing/package.xml Outdated Show resolved Hide resolved
launch_testing/setup.py Outdated Show resolved Hide resolved
launch_testing/setup.py Outdated Show resolved Hide resolved
launch_testing/package.xml Outdated Show resolved Hide resolved
@hidmic hidmic force-pushed the hidmic/import_launch_testing branch from 0f4e4ae to a8c4fa3 Compare March 27, 2019 20:14
@hidmic hidmic force-pushed the hidmic/import_launch_testing branch from a8c4fa3 to e22665a Compare April 12, 2019 20:20
@hidmic hidmic force-pushed the hidmic/import_launch_testing branch 2 times, most recently from 1a38815 to 9e7447e Compare April 16, 2019 19:28
@hidmic
Copy link
Contributor Author

hidmic commented Apr 17, 2019

I'll rebase after fixing this on Windows (which I suspect wasn't supported to begin with).

…ment_cmake.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
* Migrate launch_testing action tests into it.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/import_launch_testing branch from 9c9f40f to 8826a6a Compare April 19, 2019 02:13
@hidmic
Copy link
Contributor Author

hidmic commented Apr 22, 2019

Alright, all tests are passing but for a few unrelated, known failures! Running a linux packaging job just in case.

@dirk-thomas
Copy link
Member

What is the purpose of having the package launch_testing_ament_cmake being separate from launch_testing? Is it only because the exported CMake functions uses ament_add_test?

@hidmic
Copy link
Contributor Author

hidmic commented Apr 22, 2019

Is it only because the exported CMake functions uses ament_add_test?

Basically, yes. According to what we discussed with @wjwwood, the idea is to keep core launch packages as decoupled from ROS 2 and related build systems as possible.

@wjwwood
Copy link
Member

wjwwood commented Apr 22, 2019

Also, to keep launch_testing pure python. It could be used by other pure python things in conjunction with something like pytest without needing any cmake involvement.

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

I left some comments, but it looks good to me.

About the license, we will only left the APEX copyright on the top of the files, right?
For example in launch_testing/example_processes/exit_code_proc.

launch_testing/README.md Outdated Show resolved Hide resolved
launch_testing/README.md Outdated Show resolved Hide resolved
launch_testing/README.md Outdated Show resolved Hide resolved
launch_testing/README.md Outdated Show resolved Hide resolved
launch_testing/examples/README.md Show resolved Hide resolved
launch_testing/launch_testing/asserts/assert_output.py Outdated Show resolved Hide resolved
launch_testing/launch_testing/asserts/assert_output.py Outdated Show resolved Hide resolved
launch_testing/setup.py Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member

I forgot to add, that I'm seeing an error when I locally do:
colcon test --packages-select launch_testing
Error output:

==================================== ERRORS ====================================
ESC[31mESC[1m_________ ERROR collecting test/launch_testing/test_runner_results.py __________ESC[0m
ESC[31mImportError while importing test module '/home/ivanpauno/ros2_ws/src/ros2/launch/launch_testing/test/launch_testing/test_runner_results.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
test/launch_testing/test_runner_results.py:31: in <module>
    import mock
E   ModuleNotFoundError: No module named 'mock'ESC[0m
- generated xml file: /home/ivanpauno/ros2_ws/build/launch_testing/pytest.xml --
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!
ESC[31mESC[1m=========================== 1 error in 0.29 seconds ============================ESC[0m

It's not finding mock, but maybe it's a local problem in my environment.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Apr 25, 2019

It's not finding mock, but maybe it's a local problem in my environment.

Nope, python3-mock was missing in the package.xml. Don't forget to rosdep install.

About the license, we will only left the APEX copyright on the top of the files, right?

They authored most of it, so yes. We did some changes, not sure if we should add OSRF's too. Maybe @dirk-thomas or @wjwwood know?

@hidmic
Copy link
Contributor Author

hidmic commented Apr 25, 2019

@ivanpauno thank you for the review man! :D

@wjwwood
Copy link
Member

wjwwood commented Apr 25, 2019

It's appropriate to add OSRF to the copyright if we modified in some significant way, though that line is sort of arbitrary and if you don't add it, then it's not the end of the world.

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

I've skimmed through this and it LGTM too!

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/import_launch_testing branch from 59087a2 to 639d0da Compare April 30, 2019 14:28
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Apr 30, 2019

Alright, CI failures are unrelated. Can I get another approval after latest changes?

@hidmic hidmic merged commit cdf2924 into master Apr 30, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/import_launch_testing branch April 30, 2019 22:47
@hidmic hidmic removed the in review Waiting for review (Kanban column) label Apr 30, 2019
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.

7 participants