-
Notifications
You must be signed in to change notification settings - Fork 913
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 a very simple testcase #772
Conversation
d6cb304
to
7abe931
Compare
tools/rosbag/test/test_roundtrip.py
Outdated
def _write_simple_bag(self): | ||
from std_msgs.msg import Int32, String | ||
|
||
with rosbag.Bag('/tmp/test.bag', 'w') as bag: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a bag name which is kind of unique to this test. Otherwise it might collide with other tests being run in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to also be unique to each test_...
function? Or are those tests guaranteed to run sequentially?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default each nosetest invocation runs its tests sequentially. Separate CTests (therefore separate nosetests) will run in parallel.
But since the user can override this with e.g. NOSE_PROCESSES
I think it is good to avoid any cross talk if possible by design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed in the rewritten commits below
This exposes ros#769, but currently marks it as an expected failure
7abe931
to
aa70599
Compare
It would be great if you could provide a PR to implement the missing functionality so that these test actually pass. |
Yeah, I'm working on that PR, but I wanted to have some committed tests first in case I never get around to it |
|
||
if(CATKIN_ENABLE_TESTING) | ||
catkin_add_nosetests(test) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline please.
Please retarget this to kinetic-devel. |
I just merged #981, which fixed the failing tests on the kinetic-devel branch. Therefore this should hopefully pass now... @ros-pull-request-builder retest this please |
Looks good, thanks @eric-wieser! |
This exposes #769, but currently marks it as an expected failure. It also contains a passing test that verifies some very simple roundtrip properties.