Add tests for command line remapping by sloretz · Pull Request #268 · ros2/system_tests

sloretz · 2018-05-02T22:39:51Z

Adds a new package test_cli_remapping that checks if command line remapping arguments passed to a process result in changes to the names it uses.

sloretz · 2018-05-02T22:39:59Z

CI

Linux
Linux-aarch64
macOS
Windows

sloretz · 2018-05-10T18:12:24Z

New CI

Linux
- False positive CMake warning Cmake warning about unused variable SECURITY build_farmer#79
Linux-aarch64
macOS
Windows

mikaelarguedas

test logic looks good to me. A few nitpick comments and questions.

Question (sorry if it's obvious, I didn't follow closely the remapping implementation): Is it necessary to do everything with asyncio and coroutines ?

mikaelarguedas · 2018-05-11T03:14:18Z

test_cli_remapping/package.xml

+
+  <buildtool_depend>ament_cmake_auto</buildtool_depend>
+
+  <exec_depend>test_msgs</exec_depend>


As this is a test package that doesnt install anything, I think this is not necessary

mikaelarguedas · 2018-05-11T03:15:07Z

test_cli_remapping/package.xml

+
+  <build_depend>test_msgs</build_depend>
+  <build_depend>rclcpp</build_depend>
+  <build_depend>ament_cmake</build_depend>


Nit: alphabetical order, we usually put the buildtool_depend before the build_depends as well

1ec7f4d then removed them in 77a1140

mikaelarguedas · 2018-05-11T03:20:21Z

test_cli_remapping/CMakeLists.txt

+
+  add_executable(name_maker_rclcpp
+    test/name_maker.cpp)
+  target_compile_features(name_maker_rclcpp PRIVATE cxx_lambdas)


Is this necessary ?

Something like it is. Alternatively I can change this to set_property(TARGET name_maker_rclcpp PROPERTY CXX_STANDARD 11)

Would setting the the CMAKE_CXX_STANDARD globally like we do in the other packages do the job ?

system_tests/test_communication/CMakeLists.txt

Lines 5 to 8 in 84c035d

# Default to C++14

if(NOT CMAKE_CXX_STANDARD)

set(CMAKE_CXX_STANDARD 14)

endif()

It would, though I'm not a fan of setting things globally in cmake. I find it easier to debug stuff when I can see what is set on a specific target.

I'm not a fan of setting things globally in cmake

In general I agree.
In this specific case, we do target and rely on c++14 globally in ROS 2 so I think that setting the standard globally should be done in all packages to avoid inconsistency or surprises.

In this specific case, we do target and rely on c++14 globally in ROS 2 so I think that setting the standard globally should be done in all packages to avoid inconsistency or surprises.

Thats a good point for this package. I'll change it.

For downstream users I disagree about it meaning the standard should be set globally. Instead I think ament_target_depenencies() below should have given the target the standard required to include rclcpp headers. This would act on a package level like target_compile_features(... PUBLIC ...) does on a target level.

mikaelarguedas · 2018-05-11T03:21:54Z

test_cli_remapping/package.xml

+  <license>Apache License 2.0</license>
+
+  <build_depend>test_msgs</build_depend>
+  <build_depend>rclcpp</build_depend>


If nothing these are needed only for testing they should only be test_depends and not build depends right ?

Does test_depends say something is needed both tests are building and when tests are running?

my understanding is that test_depends are dependencies needed for building and running the tests (things you don't need to have available to build or run the package).

From the REP: When building with testing enabled, the <test_depend> packages are available for configuring and building the tests as well as running them.

mikaelarguedas · 2018-05-11T03:24:12Z

test_cli_remapping/CMakeLists.txt

+project(test_cli_remapping)
+
+find_package(ament_cmake_auto REQUIRED)
+ament_auto_find_build_dependencies()


If the build dependencies are required to compile we should specify it here (as we should error at CMake configure time if a dependency is missing and not at compile time)

I'm not sure what you mean. Where you looking for explicit find_package() calls for build dependencies? Added that in 8112882

mikaelarguedas · 2018-05-15T17:48:43Z

test_cli_remapping/test/name_maker.cpp

+
+  auto do_nothing = [](
+    const test_msgs::srv::Empty::Request::SharedPtr /*request*/,
+    test_msgs::srv::Empty::Response::SharedPtr /*response*/) -> void


I know this is used for testing only but I think misra prefers void casting unused parameters to arguments without a name

mikaelarguedas · 2018-05-15T17:49:27Z

test_cli_remapping/test/test_cli_remapping.py

+
+
+def get_environment_variable(name):
+    """Get enironment variable or raise if it does not exist."""


Nit: typo: enironment

mikaelarguedas · 2018-05-15T17:50:10Z

test_cli_remapping/test/test_cli_remapping.py

+    """Get enironment variable or raise if it does not exist."""
+    path = os.getenv(name)
+    if not path:
+        raise EnvironmentError('Missing environment variable %r' % name)


Nit: quoting the env var printed.
Also why do we need %r here ?

%r isn't necessary. I used it to make the output have quotes around name. "%s" would work too.

Sounds good, I'm fine either way 👍

%r -> "%s"
327c0fd

sloretz · 2018-05-18T15:41:32Z

Thanks for the review @mikaelarguedas

Remapping doesn't need coroutines. They are a consequence of ros2/launch#51. launch can't launch executables in a non-main thread, so launch is using the main thread to both monitor the name_maker and execute the test logic. I think I could avoid coroutines by using subprocess directly.

sloretz · 2018-05-18T15:42:44Z

Running CI Linux first

sloretz · 2018-05-18T16:31:40Z

Full CI, back in review

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas

lgtm, thanks for iterating

mikaelarguedas · 2018-06-05T15:35:51Z

test_cli_remapping/test/test_cli_remapping.py

+def get_environment_variable(name):
+    """Get environment variable or raise if it does not exist."""
+    path = os.getenv(name)
+    if not path:


Nit: pedantically this should be if path is None as some environment can exist and be set but not trigger the if (like an empty string). In the context of this test, it may be more appropriate to expect valid paths and check for path existence rather than empty string or None

I'll merge this PR as is since it adds test coverage, though in the future I hope to combine test_cli_remapping with test_cli which will be added in #274. To that end I fixed this in ec67875.

I'll create a ticket to merge the two after #274 is merged.

dirk-thomas · 2018-06-10T05:57:07Z

The tests introduced in this PR are failing on the Windows debug nightly: https://ci.ros2.org/view/nightly/job/nightly_win_deb/903/

sloretz added the in progress Actively being worked on (Kanban column) label May 2, 2018

sloretz self-assigned this May 2, 2018

sloretz mentioned this pull request May 2, 2018

Static Remapping [Meta] ros2/ros2#450

Closed

sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 10, 2018

mikaelarguedas reviewed May 15, 2018

View reviewed changes

sloretz added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 16, 2018

sloretz added 10 commits May 18, 2018 08:28

Add tests for command line remapping

1f18e06

Run launch in main thread

bf1f5e0

Remove exec_depend

564066e

Build dep alphabetical order

1ec7f4d

set c++ 14 globally

0c87b6a

explicitly find build dependencies

8112882

Remove build_depends for tests

77a1140

enironment -> environment

48609fb

%r -> "%s"

327c0fd

Cast unused variables to void

1fe0d27

sloretz force-pushed the test_cli_remapping branch from 68fb36c to 1fe0d27 Compare May 18, 2018 15:29

sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 18, 2018

mikaelarguedas self-requested a review May 31, 2018 17:08

mikaelarguedas approved these changes Jun 5, 2018

View reviewed changes

sloretz merged commit e95f4f5 into master Jun 5, 2018

sloretz deleted the test_cli_remapping branch June 5, 2018 20:48

sloretz mentioned this pull request Jun 9, 2018

Test initializing parameters from command line #274

Merged

sloretz mentioned this pull request Jun 12, 2018

Use debug python executable on windows #281

Merged


		<buildtool_depend>ament_cmake_auto</buildtool_depend>

		<exec_depend>test_msgs</exec_depend>

	# Default to C++14
	if(NOT CMAKE_CXX_STANDARD)
	set(CMAKE_CXX_STANDARD 14)
	endif()



		def get_environment_variable(name):
		"""Get enironment variable or raise if it does not exist."""

Conversation

sloretz commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented May 2, 2018

Uh oh!

sloretz commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented May 18, 2018

Uh oh!

sloretz commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented May 18, 2018

Uh oh!

mikaelarguedas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Jun 10, 2018

Uh oh!

Reviewers

Assignees

sloretz commented May 2, 2018 •

edited

Loading

sloretz commented May 10, 2018 •

edited

Loading

sloretz commented May 18, 2018 •

edited

Loading