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 rosout integration test #1924

Merged
merged 4 commits into from
Apr 27, 2020
Merged

Add rosout integration test #1924

merged 4 commits into from
Apr 27, 2020

Conversation

mm318
Copy link
Contributor

@mm318 mm318 commented Apr 3, 2020

As a follow-up to #1913 and #1915, this add a test for ensuring that the rospy.log*() functions are properly publishing log messages to the /rosout topic.

@piraka9011
Copy link

@chapulina Do you know who could review this?

@chapulina
Copy link

Looking at the referenced PRs, I believe @dirk-thomas is the best candidate. But this will most likely need to wait until after the Foxy release.

Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

LGTM

test/test_rospy/CMakeLists.txt Show resolved Hide resolved
# https://answers.ros.org/question/251194/rospy-subscriber-needs-sleep-some-time-until-the-first-message-is-received/
rospy.sleep(0.5)

log_levels = [

Choose a reason for hiding this comment

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

Consider simplifying the logic by removing the array and copying/pasting the logic for DEBUG, WARN, etc. While considered bad practice for production code, repeating code is OK for tests. This article explains why: https://testing.googleblog.com/2019/12/testing-on-toilet-tests-too-dry-make.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

'expected to receive %d log messages but got %d' % (len(log_msgs), len(self.callback_data)))

# checking contents of messages
for (callback_data, log_level, log_msg) in zip(self.callback_data, log_levels, log_msgs):

Choose a reason for hiding this comment

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

This makes the assumption that callback calls will never be reordered. Do we have such guarantee in ros_comm for local communication?

You could as well search whether the data is in the vector, to remove this assumption. It'll be closer to what I think you're trying to test here (is the data here?) vs what the test implements (is the data processed exactly in this order?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After taking the suggestion at #1924 (comment), this is no longer an issue.

@thomas-moulard
Copy link

@mm318 let me know if you need help to push this to completion!

mm318 added 2 commits April 17, 2020 10:52
Signed-off-by: Miaofei <[email protected]>
Signed-off-by: Miaofei <[email protected]>
@mm318
Copy link
Contributor Author

mm318 commented Apr 17, 2020

@mm318 let me know if you need help to push this to completion!

Yes, please! :)

@dirk-thomas
Copy link
Member

@ros-pull-request-builder retest this please

@zmichaels11
Copy link

@dirk-thomas is this PR ready?

@mm318
Copy link
Contributor Author

mm318 commented Apr 27, 2020

@ros-pull-request-builder retest this please

Hi @dirk-thomas, looks like the retest has finished and all the tests have passed, so I think this pull request is ready to be merged.

@dirk-thomas
Copy link
Member

Thanks for the new test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants