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

Add delay after node creation to ensure discovery for other rmw implementations #168

Closed
wants to merge 7 commits into from

Conversation

BMarchi
Copy link
Contributor

@BMarchi BMarchi commented Oct 28, 2019

For connext, there were two issues:

When the cli command is run,

assert cli.main(
argv=['security', 'generate_policy', os.path.join(tmpdir, 'test-policy.xml')]) == 0

it runs in another thread/context, ending up here:

subscribe_topics = get_subscriber_info(node=node, node_name=node_name)

The culprit here, is the next portion of code:
https://github.com/ros2/rmw_connext/blob/b1062f92bfbec2e85be3083144a0f6c5a599287d/rmw_connext_shared_cpp/src/node_info_and_types.cpp#L104-L139

get_discovered_participants from connext is returning 0 handlers, concluding that connext is not finding the previously created node in python.
A timer is not ideal, but for testing purposes I see no other way to ensure the node's advertising.

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 it's true that discovery latency makes it hard for us to write a robust test, but consider looping up to N times with a shorter timeout instead. It'll hopefully result in shorter run times.

…mentations

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 25, 2019

I was checking your suggestion @hidmic, but sometimes the test fails, even with my changes. Since we have to make retries and specify longer timeouts for ros2cli tests for every rmw_implementation for example, shouldn't we do the same thing here since it's using a cli feature?

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
@BMarchi BMarchi force-pushed the BMarchi/add_delay_to_test_for_node_discovery branch from 64f30c9 to 6aba460 Compare November 27, 2019 21:06
@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 27, 2019

I changed the test to start using launch_testing instead, take a look whenever you can @jacobperron , @hidmic

@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 27, 2019

  • Linux Build Status
  • Arch Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

One comment below, otherwise LGTM

from ros2cli import cli


def test_generate_policy_no_nodes(capsys):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this PR resolves the previous test failure on Windows (#143).
We probably still want to keep the TODO and skip decorator. Note, to reproduce the failure, you should also run tests for packages other than sros2 (e.g. colcon test --packages-select demo_nodes_cpp && colcon test --packages-select sros2).

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
@BMarchi
Copy link
Contributor Author

BMarchi commented Nov 28, 2019

Re run CI for linux:

  • Linux Build Status

  • Windows Build Status

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.

LGTM but for a few nits.

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
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.

LGTM!

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor

hidmic commented Dec 5, 2019

Blocked on ros2/ros2cli#419.

Re-runing CI up to sros2:

  • Linux Build Status
  • Windows Build Status

@hidmic hidmic self-assigned this Dec 6, 2019
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor

hidmic commented May 14, 2020

Rebased, refactored and used as a basis for #214. Closing.

@hidmic hidmic closed this May 14, 2020
@hidmic hidmic deleted the BMarchi/add_delay_to_test_for_node_discovery branch May 14, 2020 21:03
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.

3 participants