Test initializing parameters from command line by sloretz · Pull Request #274 · ros2/system_tests

sloretz · 2018-06-05T00:45:00Z

~~Requires #272~~
Connects to ros2/rclcpp#488

This adds tests for parameters initialized via the command line arg __params:=/path/to/yaml.file

wjwwood · 2018-06-06T08:55:53Z

41a5dcc 👍

mikaelarguedas

lgtm,

I pushed a few nitpicks on init_params_via_yaml_mikael_review feel free to cherry-pick whatever is appropriate

a3687e2: alphabetical dependency order in package.xml
f36e85d: remove unused include
d2b7b76: call unused python loop control variable '_' for readability
4b8c886: reduce test tiemout, I have not see this test take more than a couple seconds on my machine so I supposed that using the default 60 second timeout should be enough
568a6d1: use some negative values for double testing

sloretz · 2018-06-06T15:57:41Z

@mikaelarguedas merged your branch into this one; thanks for the fixes!

sloretz · 2018-06-06T20:48:26Z

Limited CI to test if windows issue is fixed:

Re-implement get_params without blocking when service is not ready

sloretz · 2018-06-06T23:51:47Z

Another rebuild to check if windows issue is fixed

sloretz · 2018-06-07T15:59:34Z

One more windows CI attempt

sloretz · 2018-06-07T21:29:31Z

Yet another windows CI attempt

sloretz · 2018-06-07T21:50:29Z

All platforms CI testing this package

Linux
Linux-aarch64
macOS
Windows

Major changes since review

sloretz · 2018-06-07T22:13:48Z

@mikaelarguedas here's a summary of the changes to this PR

Added custom NamedTemporaryFile class to util to work around issue with tempfile.NamedTemporaryFile() on windows
- A temporary file on windows can't be opened by another process without closing it, but it can't be closed without being deleted. The workaround is to create a temporary directory with a normal file inside, then delete the whole directory on __exit__().
Got rid of asycnio and coroutines.
- I was having trouble debugging the next issue, so I rewrote the tests using subprocess and threading
Every helper executable starts with a different node name
- I'm not sure what the real bug is, but I don't have time to get to root cause right now. Here's what I know.
  - Every test does the following:
    1. spawns an executable with service server /initial_params_node/get_parameters
    2. creates a client for that service
    3. calls wait_for_service()
    4. calls the service and checks values of parameters returned
    5. destroys the client
    6. shuts down the executable
  - The first test to run passes, but the second test never gets a response from the service server.
  - The issue seems to be lower level than rclpy. The second test's client handle is in the wait set but never becomes ready. I did not check with wireshark to see if the request was actually sent.
  - It's definitely a race condition, adding time.sleep(1) after calling wait_for_service() but before calling the service in the second test solves the issue.
  - Reusing the same client for all tests does not solve the issue.
  - Giving each test a different node name (and therefor a different service name) works around the issue

dirk-thomas · 2018-06-07T22:21:54Z

test_cli/test/utils.py

+        self._proc.kill()
+
+
+class NamedTemporaryFile:


The naming of this custom class is kind of confusing. It is the same as the one from the Python library but it behaves differently (doesn't return a file handle but the name). Therefore I would suggest to rename it to something more "custom" to avoid confusion.

TemporaryFileWithContent in deb5998

dirk-thomas · 2018-06-07T22:27:01Z

I'm not sure what the real bug is, but I don't have time to get to root cause right now.

Please ticket this problem to investigate it in the future.

sloretz · 2018-06-07T23:35:23Z

Please ticket this problem to investigate it in the future.

ros2/ros2#502

wjwwood

lgtm, though I wonder if this could not have been done in rclcpp itself. But maybe there's a reason I missed. Also, why a new package and not just in test_rclcpp (maybe the dependency on rclpy)? Will this package also test the python API in the same way?

wjwwood · 2018-06-09T00:07:33Z

test_cli/package.xml

+  <test_depend>ament_cmake_pytest</test_depend>
+  <test_depend>ament_lint_auto</test_depend>
+  <test_depend>ament_lint_common</test_depend>
+  <test_depend>launch</test_depend>


Seems like an unused dependency?

sloretz · 2018-06-09T00:19:44Z

@wjwwood The intent was to combine test_cli_remapping with testing parameters into test_cli, but #268 was merged first. I'll leave launch as a dependency for the moment and add it as a note to a ticket to combine the two test packages.

sloretz · 2018-06-09T00:23:53Z

Follow up ticket #279

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

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

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

sloretz added this to the bouncy milestone Jun 5, 2018

sloretz self-assigned this Jun 5, 2018

sloretz mentioned this pull request Jun 5, 2018

Add tests for command line remapping #268

Merged

sloretz added 3 commits June 5, 2018 15:44

Add test_cli package with params yaml test

dcfb8e8

test multiple parameter files via CLI

5c7e72b

Check explicitly if variable is not None

8592186

sloretz force-pushed the init_params_via_yaml branch from ec67875 to 8592186 Compare June 5, 2018 22:45

Fix inverted logic

28610d6

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

launch -> launch.legacy

41a5dcc

mikaelarguedas added 5 commits June 6, 2018 11:14

dependencies in alphabetical order

a3687e2

unused include

f36e85d

make it explicit that loop control variable is unused

d2b7b76

use default test timeout

4b8c886

test some negative double values as well

568a6d1

mikaelarguedas previously approved these changes Jun 6, 2018

View reviewed changes

Workaround windows temporary file issue

cd83f0f

Fix timeout on windows

004b64d

Re-implement get_params without blocking when service is not ready

Asyncronously get parameters

2c46cdb

Use threading instead of asyncio

4441214

dirk-thomas reviewed Jun 7, 2018

View reviewed changes

NamedTemporaryFile -> TemporaryFileWithContent

deb5998

sloretz mentioned this pull request Jun 7, 2018

Failure to call parameter service after server restarts ros2/ros2#502

Open

destroy client after spinning

9377eb6

sloretz mentioned this pull request Jun 8, 2018

Read only parameters #278

Merged

wjwwood approved these changes Jun 9, 2018

View reviewed changes

sloretz merged commit 0975759 into master Jun 9, 2018

sloretz deleted the init_params_via_yaml branch June 9, 2018 00:24

sloretz mentioned this pull request Jun 12, 2018

Use debug python executable on windows #281

Merged

Conversation

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

Uh oh!

Uh oh!

wjwwood commented Jun 6, 2018

Uh oh!

mikaelarguedas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Jun 6, 2018

Uh oh!

sloretz commented Jun 6, 2018

Uh oh!

sloretz commented Jun 6, 2018

Uh oh!

sloretz commented Jun 7, 2018

Uh oh!

sloretz commented Jun 7, 2018

Uh oh!

sloretz commented Jun 7, 2018

Uh oh!

sloretz commented Jun 7, 2018

Uh oh!

dirk-thomas Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

sloretz Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Jun 7, 2018

Uh oh!

sloretz commented Jun 7, 2018

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

wjwwood Jun 9, 2018

Choose a reason for hiding this comment

Uh oh!

sloretz commented Jun 9, 2018

Uh oh!

sloretz commented Jun 9, 2018

Uh oh!

dirk-thomas commented Jun 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sloretz commented Jun 5, 2018 •

edited

Loading

mikaelarguedas left a comment •

edited

Loading