RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
320 stars 70 forks source link

Remove cases that depends on test/rclnodejs_test_msgs under \test #856

Closed minggangw closed 2 years ago

minggangw commented 2 years ago

As these tests become unstable and cause the CI failures frequently, we decide to remove the C++ project used to verify some interactions with other ROS2 clients.

wayneparrott commented 2 years ago

Removing the rclnodejs_test_msg compile steps will definitely simplify and improve CI reliability.

re: the action tests which are dependent on the fibonacci action msg in rclnodejs_test_msgs/action we can consider using the equivalent https://github.com/ros2/test_interface_files/tree/master/action. I believe the test_interface_files pkg is bundled in the ROS releases back to foxy.

re: https://github.com/RobotWebTools/rclnodejs/blob/develop/test/rclnodejs_test_msgs/msg/StaticArrayNonPrimitives.msg any thoughts on if/how we address test-non-primitive-static-array.js

minggangw commented 2 years ago

For the test-non-primitive-static-arrary.js test, I don't find any .msg that uses a Time array in the current ROS2 release, so we may have to find something equivalent, maybe the JoyFeedbackArray.msg is suitable to replace? They are both kinds of primitives array, but we have to re-implement the case if we want to replace with it.

wayneparrott commented 2 years ago

@minggangw let me confirm that addressing this issue involves the following actions:

  1. remove test/rclnodejs_test_msgs/ folder
  2. remove script/compile_tests.js
    • note this script currently compiles: publisher_msg.cpp, subscription_msg.cpp, listener.cpp
  3. remove the pretest script in package.json that currenctly calls compile_test.js script
  4. use fibonacci msg from test_interface_files pkg in place of rclnodejs_test_msgs pkg in the following tests: test-action-graph.js test-action-client.js test-action-server.js test/types/main.ts
  5. clean up hacks in linux workflows to get around frequent env challenges when the rclnodejs_test_msgs pkg was built during testing
  6. TBD - replace test-non-primitive-static-array.js with an equivalent test using a fixed size Array

Q's:

  1. do we continue to support compiling the test/cpp/ project? if no, remove: test/cpp test/py test-cross-lang.js test-multi-nodes.js

I'll be glad to make any/all changes if you want to assign this issue to me.

minggangw commented 2 years ago

remove test/rclnodejs_test_msgs/ folder

Yes

remove script/compile_tests.js note this script currently compiles: publisher_msg.cpp, subscription_msg.cpp, listener.cpp

add_two_ints_client.cpp will also be removed,

remove the pretest script in package.json that currenctly calls compile_test.js script

Yes

use fibonacci msg from test_interface_files pkg in place of rclnodejs_test_msgs pkg in the following tests: test-action-graph.js test-action-client.js test-action-server.js test/types/main.ts

We can depend on the CI to check if we miss some test cases, as there are so many.

clean up hacks in linux workflows to get around frequent env challenges when the rclnodejs_test_msgs pkg was built during testing

Yes, finally we can get rid of it :)

TBD - replace test-non-primitive-static-array.js with an equivalent test using a fixed size Array

If it's not easy to find an alternative, we can add this case into the blocklist

do we continue to support compiling the test/cpp/ project?

I suggest we will not to add them back in the future, we will focus on the JS case only.

minggangw commented 2 years ago

@wayneparrott I have re-assigned this issue to you, please take your time, it's not urgent, thanks and stay healthy!