[rostest] Add test node if topic message is published at least once#863
[rostest] Add test node if topic message is published at least once#863dirk-thomas merged 5 commits intoros:kinetic-develfrom
Conversation
172f4f5 to
1160214
Compare
|
Please add more information to the pull request describing what the purpose of this new test is. On a first look it seems several already existing tests do cover pub / sub behavior. |
|
Updated the information. |
tools/rostest/nodes/publishtest
Outdated
| else: | ||
| return False | ||
| else: | ||
| return None |
There was a problem hiding this comment.
Can you please simplify this block of 12 lines to something shorter like this:
if self.msg:
return not self.negative
if rospy.Time.now() > self.deadline:
return self.negative
return None
|
Fixed. |
tools/rostest/nodes/publishtest
Outdated
| for param in params: | ||
| if 'name' not in param: | ||
| rospy.logerr("'name' field is required but not specified.") | ||
| sys.exit(1) |
There was a problem hiding this comment.
I don't think the test should ever call exit. Instead it should mark the test as failed using self.fail(...).
Same below.
|
Fixed. |
|
Thank you for the patch and iterating on it. |
|
Thanks. |
|
@wkentaro mind adding doc at http://wiki.ros.org/rostest/Nodes? |
|
@wkentaro Thanks! This already helped my project. |
|
@130s I'm glad to hear that. Thanks for the fix.
2017年2月18日(土) 13:08 Isaac I.Y. Saito <notifications@github.com>:
@wkentaro <https://github.com/wkentaro> Thanks! This already helped my
project.
I've updated the wiki a little. I assumed using rosparam tag is required
(i.e. no param tag).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#863 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEHFky49RvKrTqIyYCbSFG-F3lzTO6T9ks5rdm7PgaJpZM4JiZQp>
.
--
和田 健太郎 / Kentaro Wada
http://wkentaro.com
|
| from nose.tools import assert_true | ||
|
|
||
| import rospy | ||
| import rostopic |
There was a problem hiding this comment.
This package does not depend on rostopic (and it can't do so since rostopic already depends on rostest). So it can't be ensured that rostopic is being present when trying to use this node.
There was a problem hiding this comment.
So maybe one solution is we move this testing node to rostopic?
And add a warning if people use publishtest in rostest.
There was a problem hiding this comment.
So maybe one solution is we move this testing node to rostopic?
The problem with the test is being worked around in #1601.
And add a warning if people use publishtest in rostest.
I don't think there should be a warning added. It would be better to move the script to a place where it can declare its dependencies correctly. But since existing code might rely on its current location I am not sure what a backward compatible solution would look like.
Why
To serve a reusable test node for topics which has unstable topic hz, or latched.