search

ros / roslisp_common

Clone of the old roslisp_common SVN repo from code.ros.org.
8 stars 14 forks source link

Bugfix for cl-tf and a symbol export #38

Closed gaya- closed 8 years ago

gaya- commented 8 years ago

Sneaky little ... bug. Finally found you! See the description in the commit message.

airballking commented 8 years ago

@gaya- Does it make sense to make a collection of unit-tests for cl-tf to defend against regressions?

gaya- commented 8 years ago

Unit tests are always a great idea but I don't have time for it right now. Maybe a student could do this? (This particular bug is not a regression, it has been there from the very beginning, just as a note.)

gaya- commented 8 years ago

Or are you referring to regressions that could be introduced by this bugfix? I tested it in my navigation tasks on pr2_gazebo, the missing transform errors are gone, the resulting transforms seem to be all legit. Except that I cannot provide you with anything more credible, unfortunately, you would have to test it with your own applications. I could just add that this was one of the few cases where I really knew what I was doing : ).

airballking commented 8 years ago

I did not mean to imply that this fix would introduce a regression.

I was just thinking out load. As cl-tf actually is more than just an interface package, one could write unit tests for its internal functionality. Those would guard against any possible regressions introduced through subsequent commits. Also fixed bugs could be documented as unit-tests.

To re-stress, this was not a firm request. Just a thought. Maybe to keep it really small, you could start such a test-suite by adding the error you just fixed as the first test case?

gaya- commented 8 years ago

I'll write a unit test for this corner case. However, not right now, next week or last week of the next month.

airballking commented 8 years ago

So, I'm merging this now, and send the test case in a separate unit test, later?

gaya- commented 8 years ago

I don't mind.