gtagency / buzzmobile

An autonomous parade float/vehicle
MIT License
18 stars 3 forks source link

Multiple roscore #180

Closed joshuamorton closed 7 years ago

joshuamorton commented 7 years ago

This partially addresses #177, although it doesn't close it and isn't complete anyway.

(Actually it may completely address it).

Additionally, we want to fiddle with the mock_node and mock topic files to make them work on repeated use, which they currently don't.

That would mean changing from tearing the topic down to using a new subscriber, but the same node, for each test.

irapha commented 7 years ago

Another question I had was whether we needed to go back to doing pytest test/unit instead of the boxed version.. If so, we should remove the pytest-xdist dep

irapha commented 7 years ago

Local tests are very flaky. Using both pytest tests/unit and pytest -n1 tests/unit --boxed.

Also, some clean up doesn't seem to be happening. One of my tests failed and it output the message: ROS_MASTER_URI was: http://ubuntu:12053/. So I try to run roscore -p 12053 which gives me:

roscore cannot run as another roscore/master is already running. 
Please kill other roscore/master processes before relaunching.
The ROS_MASTER_URI is http://ubuntu:12053/
The traceback for the exception was written to the log file

Also, I did: killall -9 rosmaster and then roscore -p 12053 and got a warning: WARNING: ROS_MASTER_URI port [11311] does not match this roscore [12053]. But I believe this is being handled correctly in test (by looking at the code)

joshuamorton commented 7 years ago

Another question I had was whether we needed to go back to doing pytest test/unit instead of the boxed version.. If so, we should remove the pytest-xdist dep

We shouldn't need to. I'm not sure what effect it will have though. Staying shouldn't hurt.

Also, I did: killall -9 rosmaster and then roscore -p 12053 and got a warning: WARNING: ROS_MASTER_URI port [11311] does not match this roscore [12053]. But I believe this is being handled correctly in test (by looking at the code)

Two things here:

irapha commented 7 years ago

back to doing pytest test/unit

We shouldn't need to.

Well if we want parallelized tests then -n1 --boxed prevents that from happening, no?

if we want multiprocess to work, we can't use killall -9 rosmaster

I didn't imply we should use it in test. I was just noting that, after the test was done (and failed), the rosmasters weren't killed. I had to manually kill them with killall

joshuamorton commented 7 years ago

I've made a mistake, but a fruitful one:

The only issue that remains is that in rospy.init_node(), when called after the first time, the two calls to Service at the very end break. This is indicative of a larger issue though, that there really is no way to run rospy.init_node more than once in a safe manner. Essentially, if the main thread every imports rospy anywhere, we're doomed.

(what's happening by the way is that rospy is saving the get_loggers service url from the first run and using it on subsequent runs, which is problematic because the service doesn't exist because roscore has been killed. to check this, you can delete all contents of ~/.ros/log, run the tests once, and then grep for "TCP/IP", and check to see that the port number on that line is used in both tests. It should be the only port where that's the case.)

Anyway, rospy can't be used in the main thread. That means that mock_pub, mock_node, etc. need to be reworked so that they have two parts: the python objects that you interface with, and child shell friends that actually import rospy. My plan at this point is a gross amalgam of the pickle library, subprocess, and some impressive dynamism.

The upside is that this will get us about 95% of the way to #177 being all the way done, I think the only obstacles will be figuring out how to allow this new version of mock_pub and check_topic to access the port information from the outer scope.

irapha commented 7 years ago

One last question: should we still run in --boxed? Can you update ci_scripts/unittest?

And if we're not using pytest-xdist can you remove the dep from setup.py?

joshuamorton commented 7 years ago

xdist still lets us do multicprocess testing, if we want that.

irapha commented 7 years ago

Correct this comment if I'm wrong, but this:

Closes #177 entirely