Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ros2component] Add tests for ros2component #390

Open
wants to merge 9 commits into
base: rolling
Choose a base branch
from

Conversation

BMarchi
Copy link
Contributor

@BMarchi BMarchi commented Nov 1, 2019

As the title says, this PR adds tests for the ros2component cli. It's lacking tests for the standalone verb and opensplice is returning a different code error that needs further investigation.
I wasn't able to separate all the verbs because the container is kept alive for every test case and I'm not sure if we can rerun the container each time a test is run.

  • Linux Build Status
  • Arch Build Status
  • OSX Build Status

Signed-off-by: Brian Ezequiel Marchi <[email protected]>
Signed-off-by: Brian Ezequiel Marchi <[email protected]>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BMarchi consider splitting this test into multiple launch tests, each can then setup the fixture appropriately e.g. preload components into the container before testing unload or list.

@BMarchi BMarchi changed the title Add tests for ros2component [ros2component] Add tests for ros2component Nov 5, 2019
@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 5, 2019

Something to notice is that these tests don't run with opensplice. It always hangs for almost all verb conditions which ends up failing every test. I suggest to create an issue for opensplice only.

Signed-off-by: Brian Ezequiel Marchi <[email protected]>
ros2component_fixtures/CMakeLists.txt Outdated Show resolved Hide resolved
ros2component_fixtures/CMakeLists.txt Outdated Show resolved Hide resolved
ros2component/package.xml Outdated Show resolved Hide resolved
ros2component/test/test_cli_list.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_list.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_types.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_list.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_load.py Outdated Show resolved Hide resolved
ros2component_fixtures/CMakeLists.txt Outdated Show resolved Hide resolved
ros2component_fixtures/CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Brian Ezequiel Marchi <[email protected]>
Signed-off-by: Brian Ezequiel Marchi <[email protected]>
Signed-off-by: Brian Ezequiel Marchi <[email protected]>
@BMarchi BMarchi requested a review from hidmic November 11, 2019 17:27
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments!

ros2component/test/test_cli_list.py Show resolved Hide resolved
ros2component/test/test_cli_list.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_list.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_load.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_load.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_types.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_types.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_types.py Outdated Show resolved Hide resolved
ros2component/test/test_cli_unload.py Show resolved Hide resolved

// Put the message into a queue to be processed by the middleware.
// This call is non-blocking.
pub_->publish(std::move(msg));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BMarchi if you check container output, you don't even have to publish.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see why I don't have to publish, can you explain? The container process is the one who gets the output

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean you can simply check the output the container generates on component load, no need for the component to generate extra output or even have any logic beyond these tests' purposes.

Signed-off-by: Brian Ezequiel Marchi <[email protected]>
@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 14, 2019

Currently I'm dealing with the standalone verb extension. There's an issue with launch regarding the process that runs the standalone verb in opensplice . It just ignores the sigint signal, which escalates to sigterm and leaves the process alive. For fastrtps I had to resend a sigint signal in the except portion here so it can kill the process properly. I'm still investigating this behavior.

@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 21, 2019

I wasn't able to find out the reason of why standalone tests are finishing without killing the process launched. For FastRTPS I was able to wipe out the processes by re sending the SIGINT signal here when a keyboard interrupt ocurrs, making sure that the process is still alive before sending it again. But for opensplice, the process is always alive (it's ignoring the SIGINT signal completely).

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants