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

WaitForTopics: get content of messages for each topic #353

Merged

Conversation

LastStarDust
Copy link
Contributor

@LastStarDust
Copy link
Contributor Author

I don't understand why the DCO action is failing. The only commit in this pull-request already has a signed-off-by line in the description ...

@clalancette
Copy link
Contributor

I don't understand why the DCO action is failing. The only commit in this pull-request already has a signed-off-by line in the description ...

It's because it is in the wrong format. It should be:

Signed-off-by: Giorgio Pintaudi <[email protected]>

@LastStarDust LastStarDust force-pushed the feature/get-message-content branch from 6d01e84 to 8143f1d Compare February 13, 2023 13:49
@LastStarDust
Copy link
Contributor Author

@clalancette Thank you for pointing that out. I fixed it.

@LastStarDust LastStarDust force-pushed the feature/get-message-content branch 2 times, most recently from da2fa90 to 7a86ed1 Compare February 28, 2023 06:55
@LastStarDust LastStarDust changed the title Get content of topic from launch_testing_ros::WaitForTopics WaitForTopics: get content of messages for each topic Mar 1, 2023
Copy link
Contributor

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

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

Thanks for following up ! A couple of minor comments.

launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
Copy link
Contributor

@adityapande-1995 adityapande-1995 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 to me, but I'd be nice if @methylDragon or @wjwwood could also take a look.

@LastStarDust LastStarDust force-pushed the feature/get-message-content branch 3 times, most recently from d9fbc1b to 8249e34 Compare March 13, 2023 02:47
@LastStarDust
Copy link
Contributor Author

LastStarDust commented Mar 13, 2023

@adityapande-1995 The CI/CD pipeline is failing with the error:

E   ModuleNotFoundError: No module named 'launch.some_entities_type'

However, I think this is not related to any changes I have made.
It might be related to this upstream commit.

@LastStarDust
Copy link
Contributor Author

LastStarDust commented Mar 15, 2023

@adityapande-1995 @clalancette
I added a commit b5a0e09 where I fix what I think is an inconsistency in the code: if the wait method is called multiple times in a row, the same subscribers are recreated even if they already exist.

wait_for_node_object = WaitForTopics(topic_list, timeout=2.0)
wait_for_node_object.wait()
wait_for_node_object.wait() # <-- here the subscribers to the topic_list topics are recreated even if they already exist
wait_for_node_object.shutdown()

@LastStarDust
Copy link
Contributor Author

@adityapande-1995
Hello. Sorry to bother you again. I would like to know what is the review status of this pull request.
Is there anything I can do to help you in the review?

I noticed that the CI was failing probably because of a network error on the servers.
Is there a way to rerun the CI without pushing a new commit?

@adityapande-1995
Copy link
Contributor

I'll run full CI again, thanks for following up !

@adityapande-1995
Copy link
Contributor

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

@adityapande-1995
Copy link
Contributor

@LastStarDust looks like we have one small error, you can test locally using pyflakes

Signed-off-by: Pintaudi Giorgio <[email protected]>
@LastStarDust
Copy link
Contributor Author

LastStarDust commented Jun 19, 2023

@adityapande-1995
I think I fixed that error in the latest commit.

Unfortunately, when I checked the syntax with pyflakes (2.4.0 Python 3.9.13 on Windows) locally, I did not see the errors/warnings that are reported in the CI. So I am assuming you use different settings from the defaults.

So if you notice another flake8 error in the next CI, I beg you to run the linter with the correct settings and tell me exactly what is wrong, or at least tell me how to correctly set up the linter.

@adityapande-1995
Copy link
Contributor

@osrf-jenkins retest this please

@adityapande-1995
Copy link
Contributor

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

@adityapande-1995 adityapande-1995 merged commit 2d125a5 into ros2:rolling Jun 22, 2023
@LastStarDust LastStarDust deleted the feature/get-message-content branch June 23, 2023 01:18
@Ryanf55
Copy link

Ryanf55 commented Aug 15, 2023

Great addition! I was just augmenting some test fixtures to add support for this.

Is there any way this could be backported from rolling to iron and humble? In reviewing the public API changes, the arguments added to functions all have default options, so it shouldn't break consumers using class WaitForTopics. Happy to help.

@Ryanf55
Copy link

Ryanf55 commented Aug 17, 2023

@Mergifyio backport iron
@Mergifyio backport humble

@mergify
Copy link

mergify bot commented Aug 17, 2023

backport iron

❌ Command disallowed due to command restrictions in the Mergify configuration.

  • sender-permission>=write

@Ryanf55
Copy link

Ryanf55 commented Aug 17, 2023

@adityapande-1995 Would you be able to run the backport commands for this PR to iron and humble? I do not have permission. Much appreciated!

clalancette added a commit that referenced this pull request Aug 24, 2023
In commit 0c081a7
(#360) we increased
the timeout to 10 seconds to accomodate slower discovery.
However, this was partially reverted in
2d125a5
(#353).

Restore the 10 second timeout to avoid flakes on the
buildfarm.

Signed-off-by: Chris Lalancette <[email protected]>
clalancette added a commit that referenced this pull request Aug 28, 2023
In commit 0c081a7
(#360) we increased
the timeout to 10 seconds to accomodate slower discovery.
However, this was partially reverted in
2d125a5
(#353).

Restore the 10 second timeout to avoid flakes on the
buildfarm.

Signed-off-by: Chris Lalancette <[email protected]>
@mjcarroll
Copy link
Member

https://github.com/Mergifyio backport iron
https://github.com/Mergifyio backport humble

Copy link

mergify bot commented Jan 16, 2024

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 16, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)
adityapande-1995 pushed a commit that referenced this pull request Jan 16, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)
Signed-off-by: Aditya Pande <[email protected]>
@adityapande-1995
Copy link
Contributor

https://github.com/Mergifyio backport humble

Copy link

mergify bot commented Jan 16, 2024

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 16, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)

# Conflicts:
#	launch_testing_ros/launch_testing_ros/wait_for_topics.py
#	launch_testing_ros/test/examples/wait_for_topic_launch_test.py
adityapande-1995 pushed a commit that referenced this pull request Jan 16, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)

# Conflicts:
#	launch_testing_ros/launch_testing_ros/wait_for_topics.py
#	launch_testing_ros/test/examples/wait_for_topic_launch_test.py
Signed-off-by: Aditya Pande <[email protected]>
adityapande-1995 pushed a commit that referenced this pull request Jan 17, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)

# Conflicts:
#	launch_testing_ros/launch_testing_ros/wait_for_topics.py
#	launch_testing_ros/test/examples/wait_for_topic_launch_test.py
Signed-off-by: Aditya Pande <[email protected]>
mjcarroll pushed a commit that referenced this pull request Jan 20, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346

---------


(cherry picked from commit 2d125a5)

Signed-off-by: Pintaudi Giorgio <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Co-authored-by: Giorgio Pintaudi <[email protected]>
mjcarroll pushed a commit that referenced this pull request Jan 20, 2024
… (#389)

* `WaitForTopics`: get content of messages for each topic (#353)

* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)

# Conflicts:
#	launch_testing_ros/launch_testing_ros/wait_for_topics.py
#	launch_testing_ros/test/examples/wait_for_topic_launch_test.py
Signed-off-by: Aditya Pande <[email protected]>

* Resolve conflicts (#390)

Signed-off-by: Ryan Friedman <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>

---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Ryan Friedman <[email protected]>
Co-authored-by: Giorgio Pintaudi <[email protected]>
Co-authored-by: Ryan <[email protected]>
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.

7 participants