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

Adds a --max-wait-time option to ros2 topic pub #800

Merged
merged 13 commits into from
Feb 24, 2023

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Feb 3, 2023

Supercedes #797

When we run ros2 topic pub -w 1 ... we can wait for a fixed number
of connections prior to shutting down. This is useful in many scenarios,
however when writing automated tests it may be useful to time out if the
correct number of connections are not recieved in a given amount of
time. This PR adds a --max-wait-time option which allows users to set
a timeout on how long to wait for subscribers.

Signed-off-by: Arjo Chakravarty arjo@openrobotics.org

When we run `ros2 topic pub -w 1 ...` we can wait for a fixed number
of connections prior to shutting down. This is useful in many scenarios,
however when writing automated tests it may be useful to time out if the
correct number of connections are not recieved in a given amount of
time. This PR adds a `--max-wait-time` option which allows users to set
a timeout on how long to wait for subscribers.

Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@clalancette
Copy link
Contributor

Just as an FYI, in general we prefer rebases of old PRs rather than new PRs. That is, it would have been preferable here to keep #797 open, and just rebase it, rather than close it and make this new one. That keeps all of the conversations in the same place and ensures it is easier to follow later.

That said, it is already done here, so let's just go forward with this one.

ros2topic/ros2topic/verb/pub.py Outdated Show resolved Hide resolved
ros2topic/test/test_echo_pub.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/verb/pub.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/verb/pub.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/verb/pub.py Outdated Show resolved Hide resolved
ros2topic/ros2topic/verb/pub.py Outdated Show resolved Hide resolved
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 force-pushed the arjo/feat/max_wait_time_clean branch from 770ce08 to 58a16d1 Compare February 6, 2023 06:37
@arjo129 arjo129 requested review from clalancette and removed request for gbiggs and audrow February 6, 2023 13:30
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

The code that is here looks OK to me. However, I think we should have two additional tests:

  1. A test that shows that using --max-wait-time-secs without --wait-matching-subscriptions immediately fails.
  2. A test that shows that with a subscriber, we actually do eventually break out of the loop and publish something.

Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Copy link
Contributor Author

arjo129 commented Feb 7, 2023

Added tests in ccb2864

@arjo129 arjo129 requested a review from clalancette February 7, 2023 11:13
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 requested a review from clalancette February 9, 2023 00:33
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good with green CI.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

@arjo129 It looks like one of the new tests is triggering CI. Can you take a look?

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@arjo129
Copy link
Contributor Author

arjo129 commented Feb 22, 2023

Is it possible to check CI again? I've changed the problematic test case.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor

Another try with flake8 warnings fixed:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

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