Fix test_message_conversion.py #645

kenji-miyake · 2021-09-24T15:05:41Z

Public Changes

None

Description

Fixed test_message_conversion.py.
Since it has some bugs in the application code, I'll fix them in another PR #646.

Related: #643

amacneil · 2021-09-28T16:45:27Z

Glad to see #651 caught some new missing test dependencies.

fyi @jtbandes

amacneil

thanks! needs rebase before merging

kenji-miyake · 2021-10-09T16:42:44Z

@amacneil Thank you! How do we merge this?

Close this and merge Fix message conversion #646 instead
Apply fixes in Fix message conversion #646 first and add tests
Merge this (temporarily breaks CI), quickly rebase and merge Fix message conversion #646

jtbandes · 2021-10-11T18:24:40Z

rosbridge_library/test/internal/test_message_conversion.py

+        for msg in [{"data": data_value}]:
+            inst = ros_loader.get_message_instance(msgtype)
+            c.populate_instance(msg, inst)
+            self.assertEqual(inst.data, bytes([data_value]))


I'm not sure this is right. std_msgs/Byte holds a single byte, why should it be a bytes instance?

I'm not so familiar with this, but as far as I know, it's because rclpy defines so.

https://design.ros2.org/articles/generated_interfaces_python.html

In [1]: from example_interfaces.msg import Byte In [2]: Byte().data Out[2]: b'\x00' In [3]: Byte().data == bytes([0]) Out[3]: True In [4]: Byte().data == 0 Out[4]: False

jtbandes · 2021-10-11T18:26:31Z

As for how to merge this PR: I think it's fine to merge this PR first (once approved), just disable the failing tests using unittest.skip. Then the skip can be removed in #646.

jtbandes · 2021-10-11T18:28:27Z

rosbridge_library/test/internal/test_message_conversion.py

+        msg = {"sec": 3, "nanosec": 5}
+        self.do_test(msg, "builtin_interfaces/Time")

-        msg = {"times": [{"secs": 3, "nsecs": 5}, {"secs": 2, "nsecs": 7}]}
+        msg = {"times": [{"sec": 3, "nanosec": 5}, {"sec": 2, "nanosec": 7}]}


Does this mean the JSON format of time/duration fields is changing for ROS 2? Will clients need to update their code? I think we need to maintain backward compatibility with the secs/nsecs format if that is the format existing JSON clients are expecting.

Does this mean the JSON format of time/duration fields is changing for ROS 2?

As a test spec, yes.
As an application spec, no. It's already changed.

To test this, please run the following commands.

Case 1. Working

// Terminal 1 $ ros2 run rosbridge_server rosbridge_websocket // normal outputs // Terminal 2 $ curl -sL https://github.com/tier4/rosbridge_suite/raw/add-test-scripts/test_scripts/time.py | python publish: {'sec': 1, 'nanosec': 2} subscribe: {'sec': 1, 'nanosec': 2} publish: {'sec': 1, 'nanosec': 2} subscribe: {'sec': 1, 'nanosec': 2}

Case 2. Not working

// Terminal 1 $ ros2 run rosbridge_server rosbridge_websocket [ERROR 1634483257.554689935] [rosbridge_websocket]: [Client 4] [id: publish:/time:3] publish: 'Time' object has no attribute 'secs' [ERROR 1634483258.559444873] [rosbridge_websocket]: [Client 4] [id: publish:/time:4] publish: 'Time' object has no attribute 'secs' // Terminal 2 $ curl -sL https://github.com/tier4/rosbridge_suite/raw/add-test-scripts/test_scripts/time_old_format.py | python publish: {'secs': 1, 'nsecs': 2} publish: {'secs': 1, 'nsecs': 2}

Even though it's already changed, we could still choose to change it back (and we should bump the major version of the package if it's not backward compatible). Do you think users of the library will be confused by the difference in behavior between ROS 1/2? If someone upgrades from ROS 1 to ROS 2 their websocket client will start receiving values. Should we change it back to secs/nsecs or at least change the publish side to accept both forms?

cc @defunctzombie @amacneil for thoughts.

@jtbandes I'll leave the decision to the maintainers, but my personal (and selfish) thought is, since our company has completely moved to ROS2, either one is fine.

Do you think users of the library will be confused by the difference in behavior between ROS 1/2?

Yes, I remember we had to fix some web application code for ROS2, but it wasn't so big confusion.

But anyway, I think clients should change the data format when they receive the data because rosbridge outputs the same data for each client. Is this correct or does rosbridge change output data depending on the client ROS version?

I'm not following the comments in this thread. Is the statement that rosbridge currently outputs sec/nsec and this is confusing for ros2 users and we should change to sec/nanosec? Or that rosbridge outputs sec/nanosec and this is confusing for ros1 users?

What is the specific change (if any) being requested of the api/protocol?

Currently it outputs secs/nsecs for ROS 1 and sec/nanosec for ROS 2. This PR just changes the test.

If this is just changing a test then we should merge the PR to have tests and tackle API changes in separate discussions.

jtbandes · 2021-12-09T23:48:35Z

Can you please rebase on #686 ? These newly added tests were not running in CI, and they might need more changes to be able to pass CI.

kenji-miyake · 2021-12-10T01:56:21Z

Hmm, It seems the test is running at least in this PR. @jtbandes Do you know why?
https://github.com/RobotWebTools/rosbridge_suite/runs/3867023090?check_suite_focus=true

But anyway, I'll rebase related PRs later.

jtbandes · 2021-12-10T02:58:37Z

Hm, maybe only some tests are affected, not sure why. I just merged that PR so you should be able to rebase on ros2 now. Thanks for your patience!

kenji-miyake · 2021-12-10T03:47:25Z

Rebased. The tests won't pass because there is no logic change.

jtbandes · 2021-12-10T03:49:54Z

Yep, that makes sense. Can you use @unittest.skip or @pytest.mark.xfail to disable the tests that are failing? Then we can merge this PR, and you can re-enable them in the other PR.

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

kenji-miyake · 2021-12-10T05:08:48Z

@jtbandes Yes it's possible, but before that, we have to agree with the test specifications around Time related to this discussion.
#645 (comment)

Should I add tests for both secs/nsecs and sec/nanosec?

jtbandes · 2021-12-10T05:14:32Z

I think it's fine to keep sec/nanosec behavior for ROS 2 because that's what the builtin_interfaces/Time and Duration use. That's what you were already planning to do with these PRs, right?

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

kenji-miyake · 2021-12-10T05:25:45Z

@jtbandes That's right, but I'm not sure it's enough.
For example, if there are only tests for sec/nanosec, the test passes even if I completely remove secs/nsecs from the code.
Like 21ecfe6 in #646

Anyway, I skipped invalid tests and the tests passed. Is that okay for you?

jtbandes · 2021-12-10T05:34:29Z

the test passes even if I completely remove secs/nsecs from the code

I see. Just trying to understand the current behavior... is rosbridge currently accepting both secs/nsecs and sec/nanosec for any time/duration fields in op: publish? If so, removing it would be a breaking change and I see why we would want to cover it in the test. In that case keeping the current behavior (accepting both secs/nsecs and sec/nanosec) seems fine, otherwise we would need to bump the package major version (and/or add some features to the protocol for clients to detect the server version).

On the subscriber side, is it right that rosbridge will always send sec/nanosec to subscribers (since the initial ros2 conversion)?

Anyway, I think these issues can be addressed in #646. If you don't mind, I can merge this PR now.

By the way, if you would like to discuss any of these issues/PRs in a faster communication channel, we have a Slack workspace at http://foxglove.dev/slack with a #ros2 channel. Feel free to ping us anytime in there 🙂 I'm also a member of the ros discord server.

kenji-miyake · 2021-12-10T05:53:29Z

Anyway, I think these issues can be addressed in #646. If you don't mind, I can merge this PR now.

@jtbandes Yes, I'm okay if maintainers are okay with test specs. (Oh, merged while I'm writing, thank you!)
Actually, #646 has some mixed fixes, so I'll split them into separated small problems.

is rosbridge currently accepting both secs/nsecs and sec/nanosec for any time/duration fields in op: publish?

I'm sorry, I'm not familiar with this behavior. 🥲

jtbandes · 2021-12-10T05:54:55Z

is rosbridge currently accepting both secs/nsecs and sec/nanosec for any time/duration fields in op: publish?

I'm sorry, I'm not familiar with this behavior. 🥲

Me neither 😆 But I thought maybe that's what this code is trying to do (convert a JSON message from the ws client into a ROS time/duration msg).

kenji-miyake mentioned this pull request Sep 24, 2021

Fix message conversion #646

Closed

kenji-miyake marked this pull request as draft September 24, 2021 15:07

kenji-miyake mentioned this pull request Sep 24, 2021

Fix test settings for rosbridge_library #643

Merged

kenji-miyake force-pushed the fix-rostest-message-conversion branch from a78ef21 to d13769b Compare September 28, 2021 06:02

kenji-miyake force-pushed the fix-rostest-message-conversion branch from 0faf609 to b2d8d8d Compare October 8, 2021 08:44

kenji-miyake marked this pull request as ready for review October 8, 2021 09:22

amacneil approved these changes Oct 8, 2021

View reviewed changes

jtbandes reviewed Oct 11, 2021

View reviewed changes

kenji-miyake force-pushed the fix-rostest-message-conversion branch from f8a543a to f914541 Compare December 10, 2021 03:43

Kenji Miyake added 11 commits December 10, 2021 13:56

Enable test_message_conversion

3edf533

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Fix rospy

e42e79b

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Fix Header structure

53fe6a7

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Fix serialization

0d78a3c

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Fix time and duration

873fe60

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Fix byte array

5d140ac

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Replace message_conversion.string_types

7f3ecd6

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Remove time=now

d32b50b

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Fix tests for byte

72c5b14

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Remove unnecessary rclpy

91991ac

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

Skip non-passing tests

71dc3e0

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

kenji-miyake marked this pull request as draft December 10, 2021 05:05

Disable array tests for Time and Duration

be45b8e

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>

kenji-miyake force-pushed the fix-rostest-message-conversion branch from f914541 to be45b8e Compare December 10, 2021 05:19

kenji-miyake marked this pull request as ready for review December 10, 2021 05:27

jtbandes merged commit cf57ed9 into RobotWebTools:ros2 Dec 10, 2021

kenji-miyake deleted the fix-rostest-message-conversion branch December 10, 2021 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test_message_conversion.py #645

Fix test_message_conversion.py #645

kenji-miyake commented Sep 24, 2021 •

edited

Loading

amacneil commented Sep 28, 2021

amacneil left a comment

kenji-miyake commented Oct 9, 2021

jtbandes Oct 11, 2021

kenji-miyake Oct 17, 2021 •

edited

Loading

jtbandes commented Oct 11, 2021

jtbandes Oct 11, 2021

kenji-miyake Oct 17, 2021 •

edited

Loading

jtbandes Nov 16, 2021

kenji-miyake Nov 17, 2021

defunctzombie Dec 10, 2021

jtbandes Dec 10, 2021

defunctzombie Dec 10, 2021

jtbandes commented Dec 9, 2021

kenji-miyake commented Dec 10, 2021

jtbandes commented Dec 10, 2021

kenji-miyake commented Dec 10, 2021

jtbandes commented Dec 10, 2021

kenji-miyake commented Dec 10, 2021

jtbandes commented Dec 10, 2021

kenji-miyake commented Dec 10, 2021 •

edited

Loading

jtbandes commented Dec 10, 2021 •

edited

Loading

kenji-miyake commented Dec 10, 2021

jtbandes commented Dec 10, 2021

Fix test_message_conversion.py #645

Fix test_message_conversion.py #645

Conversation

kenji-miyake commented Sep 24, 2021 • edited Loading

amacneil commented Sep 28, 2021

amacneil left a comment

Choose a reason for hiding this comment

kenji-miyake commented Oct 9, 2021

jtbandes Oct 11, 2021

Choose a reason for hiding this comment

kenji-miyake Oct 17, 2021 • edited Loading

Choose a reason for hiding this comment

jtbandes commented Oct 11, 2021

jtbandes Oct 11, 2021

Choose a reason for hiding this comment

kenji-miyake Oct 17, 2021 • edited Loading

Choose a reason for hiding this comment

jtbandes Nov 16, 2021

Choose a reason for hiding this comment

kenji-miyake Nov 17, 2021

Choose a reason for hiding this comment

defunctzombie Dec 10, 2021

Choose a reason for hiding this comment

jtbandes Dec 10, 2021

Choose a reason for hiding this comment

defunctzombie Dec 10, 2021

Choose a reason for hiding this comment

jtbandes commented Dec 9, 2021

kenji-miyake commented Dec 10, 2021

jtbandes commented Dec 10, 2021

kenji-miyake commented Dec 10, 2021

jtbandes commented Dec 10, 2021

kenji-miyake commented Dec 10, 2021

jtbandes commented Dec 10, 2021

kenji-miyake commented Dec 10, 2021 • edited Loading

jtbandes commented Dec 10, 2021 • edited Loading

kenji-miyake commented Dec 10, 2021

jtbandes commented Dec 10, 2021

kenji-miyake commented Sep 24, 2021 •

edited

Loading

kenji-miyake Oct 17, 2021 •

edited

Loading

kenji-miyake Oct 17, 2021 •

edited

Loading

kenji-miyake commented Dec 10, 2021 •

edited

Loading

jtbandes commented Dec 10, 2021 •

edited

Loading