replace info with launch testing #351

Karsten1987 · 2020-04-04T21:03:32Z

fixes #271 by replacing the end to end tests in rosbag2_tests with launch_testing.

Signed-off-by: Karsten Knese [email protected]

Signed-off-by: Karsten Knese <[email protected]>

rosbag2_tests/test/rosbag2_tests/test_rosbag2_info_end_to_end.py

Signed-off-by: Karsten Knese <[email protected]>

Karsten1987 · 2020-04-06T23:17:53Z

to get an idea about windows:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Karsten Knese <[email protected]>

Karsten1987 · 2020-04-07T00:28:30Z

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2020-04-07T01:42:26Z

so this doesn't sound good:

18:38:53 3: [WARNING] [ros2-1]: 'SIGINT' sent to process[ros2-1] not supported on Windows, escalating to 'SIGTERM'
18:38:53 3: [INFO] [ros2-1]: sending signal 'SIGTERM' to process[ros2-1]

@hidmic the test_play_end_to_end.py is timing out. It might be related to the above warning/error. Do you have any idea on how to work around this?

Any help is appreciated. I'd like to have this sorted out before digging deeper into porting all tests to launch testing.

Signed-off-by: Karsten Knese <[email protected]>

hidmic

About SIGINT not being sent on Windows, that is (unfortunately) expected. There is no equivalent in Windows, and thus launch (not the testing framework but the process management infrastructure itself) escalates to a SIGTERM equivalent (see here).

@wjwwood do you recall why CTRL_C_EVENT was never an option in Windows? I recall it wouldn't work right away, but that's as far as my memory goes.

hidmic · 2020-04-07T13:07:19Z

rosbag2_tests/test/rosbag2_tests/test_rosbag2_info_end_to_end.py

+             r'\| Count: 3 \| Serialization Format: cdr\n'
+             r'\s+Topic: /array_topic \| Type: test_msgs/Arrays '
+             r'\| Count: 4 \| Serialization Format: cdr'))
+        launch_testing.asserts.assertInStdout(proc_output, p, rosbag_info_process)


@Karsten1987 FYI you can also assert on command.output, and there's launch_testing.tools.output.expect_output to aid that if you want.

but expect_output does not accept a regex pattern, does it?

Yeap, it does. All these assertIn* functions use expect_output under the hood.

hidmic · 2020-04-07T13:08:20Z

rosbag2_tests/test/rosbag2_tests/test_rosbag2_info_end_to_end.py

+        with launch_testing.tools.launch_process(
+              launch_service, rosbag_info_process, proc_info, proc_output) as command:
+            assert command.wait_for_shutdown(timeout=3)
+            assert command.exit_code == launch_testing.asserts.EXIT_OK


@Karsten1987 nit: I'd move this statement outside the context manager scope

how do I access the exit code if the command is out of scope?

command doesn't leave scope after the with statement is over :)

hidmic · 2020-04-07T13:11:11Z

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.py

+        try:
+            shutil.rmtree(dir_path)
+        except OSError as e:
+            print("Error: %s : %s" % (dir_path, e.strerror))


@Karsten1987 you may want to use setUp() and tearDown() methods to create and destroy temporary resources.

hidmic · 2020-04-07T13:12:15Z

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.py

+        with launch_testing.tools.launch_process(
+              launch_service, rosbag_record_process, proc_info, proc_output) as command:
+            time.sleep(10)
+        proc_info.assertWaitForShutdown(rosbag_record_process, timeout=5)


@Karsten1987 FYI you can use command.wait_for_shutdown too.

hidmic · 2020-04-07T13:39:46Z

@hidmic the test_play_end_to_end.py is timing out. It might be related to the above warning/error. Do you have any idea on how to work around this?

Hmm, I looked at it for a while but I don't see how it got into that state. Exit codes are unrelated. Can you reproduce in a Windows box? If you run launch_test with -v that may help us understand what is happening.

wjwwood · 2020-04-07T13:59:41Z

@wjwwood do you recall why CTRL_C_EVENT was never an option in Windows? I recall it wouldn't work right away, but that's as far as my memory goes.

No I don’t remember the details, only that it is asymmetric between windows and Linux.

Signed-off-by: Karsten Knese <[email protected]>

Karsten1987 · 2020-04-09T03:00:02Z

Are you guys aware of any workarounds for the ctrl-c dilemma? I just played around locally with stuff like shown here: https://gist.github.com/rdp/f51fb274d69c5c31b6be

Which works pretty decently and rosbag behaves accurately, meaning it receives the ctrl-c and writes the bagfiles correctly. I found the equivalent in python using ctypes:
https://stackoverflow.com/a/58802146/4583130

Would you have an idea if that route is somewhat feasible?

Karsten1987 · 2020-04-10T19:35:33Z

After talking with @hidmic I've tried a couple of more things (CTRL_C_EVENT instead of SIGTERM, playing around with creationflags (https://docs.microsoft.com/en-us/windows/win32/procthread/process-creation-flags)) but that all seemed to lead into a deep rabbit hole which is most likely not worth it.

In order to stabilize the end to end tests, I have a couple of ideas I'd like to run by you (i.e. @emersonknapp, @piraka9011, @zmichaels11).

introduce a new CLI option which dictates the duration of recording (could possibly also be used for replaying). That would allow the record e2e tests to automatically stop after n seconds and we wouldn't need to rely on a custom ctrl-c event.
convert the internal Rosbag2Node into a lifecycle node. I think this could help us overcome parts of the flaky discovery problems we encounter especially in the replay tests. In the configure step of the lifecycle node, we could prepare and advertise the publishers on the system. Likewise, the subscriptions in the test can be started. After that, the activate step really only triggers the call to publish.
similar to the approach above, I've started work a while ago on a CLI option called --paused where the rosbag would start to prepare everything, but don't publish. Either by a service call or spacebar the replay would start. As a gut feeling, I am in favor of the lifecycle node though.

Any opinions?

piraka9011 · 2020-04-10T19:45:42Z

I too played around with this ctrl-c business on windows (See this branch, specifically, this file). However, my approach kills colcon test when it attempts to raise ctrl-c (I opened an issue here).

I came to the conclusion that the right way to go about this is to introduce keyboard commands to control the rosbag2 node, specifically things like s/stop, q/quit and p/pause.

The lifecycle node approach is also good, it allows a user to programmatically control the rosbag2 node. I think keyboard/cli/lifecycle node approaches are all valid and we should decide what is the most useful approach (probably lifecycle).

Your first approach is the fastest to develop IMO and a good short term solution to the issue.

piraka9011 · 2020-04-10T19:49:18Z

FYI, Also played around with the windows creation flags, trying out different combinations one by one...
Unfortunately it did not resolve the issue 🙃

zmichaels11 · 2020-04-10T21:12:46Z

Instead of invoking the ros2 cli, can the tests instead just invoke the commands that the cli runs?

Karsten1987 · 2020-04-10T22:47:57Z

I think keyboard/cli/lifecycle node approaches are all valid and we should decide what is the most useful approach (probably lifecycle).

I don't see them as mutual exclusive. I could see how the lifecycle generally offers a way to steer the communication and the keyboard inputs are really only a trigger to the state transitions of that lifecycle. That would make sense to me.

However, my approach kills colcon test when it attempts to raise ctrl-c

I ran into the same thing. The closest I came to a solution was trying to use CREATE_NEW_CONSOLE, but this also didn't really yield any results.

Instead of invoking the ros2 cli, can the tests instead just invoke the commands that the cli runs?

I guess we could tackle the e2e tests starting from rosbag2_transport. Cause, really, the CLI is only forwarding the command line options to the constructor of the transport class.

emersonknapp · 2021-10-28T22:10:30Z

closing as stale. can reopen if anyone wants to take on this work

replace info with launch testing

711a42e

Signed-off-by: Karsten Knese <[email protected]>

Karsten1987 commented Apr 4, 2020

View reviewed changes

rosbag2_tests/test/rosbag2_tests/test_rosbag2_info_end_to_end.py Outdated Show resolved Hide resolved

rosbag2_tests/test/rosbag2_tests/test_rosbag2_info_end_to_end.py Outdated Show resolved Hide resolved

Karsten1987 added 3 commits April 5, 2020 13:47

use regular expression for info

524fe77

Signed-off-by: Karsten Knese <[email protected]>

fix transport test

e9b48ae

Signed-off-by: Karsten Knese <[email protected]>

wip play end to end

512c6fb

Signed-off-by: Karsten Knese <[email protected]>

Karsten1987 force-pushed the end_to_end_launch_testing branch from b73d0cd to 512c6fb Compare April 6, 2020 17:55

Karsten1987 added 3 commits April 6, 2020 11:37

add bag file for replay

9c377a8

Signed-off-by: Karsten Knese <[email protected]>

regex for finding messages in ros2 topic echo

46e82ba

Signed-off-by: Karsten Knese <[email protected]>

make process variables local

2b6dac8

Signed-off-by: Karsten Knese <[email protected]>

Karsten1987 added 2 commits April 6, 2020 16:57

add ros2topic dependency

0974b84

Signed-off-by: Karsten Knese <[email protected]>

remove std msgs dependency

0d7f8fc

Signed-off-by: Karsten Knese <[email protected]>

wip testing record end to end

7538525

Signed-off-by: Karsten Knese <[email protected]>

hidmic reviewed Apr 7, 2020

View reviewed changes

address review comments

88906fb

Signed-off-by: Karsten Knese <[email protected]>

piraka9011 mentioned this pull request Apr 10, 2020

Fix rosbag2_tests resource files and play_end_to_end test #362

Merged

emersonknapp closed this Oct 28, 2021

MichaelOrlov deleted the end_to_end_launch_testing branch July 13, 2024 07:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace info with launch testing #351

replace info with launch testing #351

Karsten1987 commented Apr 4, 2020

Karsten1987 commented Apr 6, 2020

Karsten1987 commented Apr 7, 2020

Karsten1987 commented Apr 7, 2020

hidmic left a comment

hidmic Apr 7, 2020

Karsten1987 Apr 7, 2020

hidmic Apr 7, 2020

hidmic Apr 7, 2020

Karsten1987 Apr 7, 2020

hidmic Apr 7, 2020

hidmic Apr 7, 2020

hidmic Apr 7, 2020

hidmic commented Apr 7, 2020

wjwwood commented Apr 7, 2020

Karsten1987 commented Apr 9, 2020

Karsten1987 commented Apr 10, 2020

piraka9011 commented Apr 10, 2020 •

edited

Loading

piraka9011 commented Apr 10, 2020

zmichaels11 commented Apr 10, 2020

Karsten1987 commented Apr 10, 2020

emersonknapp commented Oct 28, 2021

replace info with launch testing #351

replace info with launch testing #351

Conversation

Karsten1987 commented Apr 4, 2020

Karsten1987 commented Apr 6, 2020

Karsten1987 commented Apr 7, 2020

Karsten1987 commented Apr 7, 2020

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Apr 7, 2020

wjwwood commented Apr 7, 2020

Karsten1987 commented Apr 9, 2020

Karsten1987 commented Apr 10, 2020

piraka9011 commented Apr 10, 2020 • edited Loading

piraka9011 commented Apr 10, 2020

zmichaels11 commented Apr 10, 2020

Karsten1987 commented Apr 10, 2020

emersonknapp commented Oct 28, 2021

piraka9011 commented Apr 10, 2020 •

edited

Loading