ROS Communication Testing: Add Tests for Topic Creation and Message Chain Verification

[Yunus-Selim]

closes #55

This code aims to test for the existence of topics and the existence of topic messages, to make sure there is communication between nodes. But it might have some bugs for the other config files. I pull request it for early analysis before the meeting.

And I have a few questions about the code:

1. Do you want me to use something else like correctness of messages for the assertion instead of checking the existence of topic messages?
2. Should I test for the RobotStatePublisher node for this task?
3. Should I complete the TODOs in the test code? Do you think that they are necessary?

[luca-byte]

1. Yes, but in separate test files, also we may want to check the frequency of the messages on the topic. There is a Sampling frequency in the remote controller (remote_controller) 80ms and one for the telemetry/feedback 100Hz (PicoLowLevel)
2. No
3. Yes, by using the data from YAML files, harrdcoded.

Implement the Canbus in python to test feedback reception from the communication node. Also makes sense to test for failures

[luca-byte]

Excellent work!! 💯

There are some problems (namely the impossibility to run tests with colcon and the communication node tests) and I left some considerations about those.

Moreover I think that the tests/classes/functions could benefit from having some docstrings/comments that explain briefly the test objective and the method used to test it.

I also kindly ask you to write a more detailed description of the approach (values selected for the test, how are oracles derived, ...) on Outline, in the section I just created.

[luca-byte]

I noticed that with the pytest mark the tests are visualized as only one test, thus making it difficult to see if one or more tests are failing.

Moreover I came to the conclusion that my suggestion to add the script was not a good idea for many reasons (simply because colcon does not allow to use the stdin, ...) so probably you will have to remove it (sorry 😁) however I still believe that we should skip tests when the interface is not available (and your approach to check the ip link output is the best onne in my opinion).

As per the first point however, I would put the test that require the CAN interface in separate files, so that it is clear that they were skipped even when merged in one test by launch_test in colcon

Regarding the socketcan shutdown I intended shutting it down at the end of the test, since now (with the new implementation, a failure to setup it blocks also tests which are independent from it. I meant something like

def test_with_canbus(...):
  # setup canbus
  ...
  canbus.shutdown(...)

or something similar, but if you agree on separating the tests depending on the CAN we can do as you did in these tests.

[luca-byte]

Regarding documentation: 👌

[luca-byte]

Great job 💯

I noticed that between this PR and the ones testing the nodes, there is a lot of duplicated code (launch, ...). Would it be possible to merge together that code to improve readability and reduce duplicated code, maybe abstracting it in some classes/functions?

[luca-byte]

I do not like much this approach because we are polluting the production code with code which is never executed by the robot (unlike launch utilities). Isn't it possible to include the file from the same folder (if flake complains in the inclusion you can silence it using noqa on the offending line)

After that I will approve the PR. Great job 🎯

[luca-byte]

LGTM

In merging, the testing utilities of this PR can be merged with Yusuf's one?

[Yunus-Selim]

It can be merged without problem now.

[luca-byte]

LGTM