improved rospy core logger test. #1144
Conversation
now allowing to customize out/err streams for RosStreamHandler
|
I also modified the Guideline : keep the core as pure (functional) as possible, push side-effects to the side. |
There was a problem hiding this comment.
This is a nice improvement to the tests. Thank you for your effort.
Throttled loggers need node to be initialized (to get the time...) so we might test them in a rostest instead (unless we can somehow remove that constraint)
test_rosgraph might be a good location for such tests.
| rospy.core.configure_logging("/") | ||
| self.fail("configure_logging should not accept a the root namespace as the node_name param") | ||
| except: pass | ||
| rospy.core.configure_logging("/node/name") |
There was a problem hiding this comment.
Why has this test been removed?
There was a problem hiding this comment.
somehow it didn't find that bug : https://github.com/ros/ros_comm/pull/1144/files#diff-84739190beb4f06aecd26a816f126e29L350
and it s now tested in context there: https://github.com/ros/ros_comm/pull/1144/files#diff-719224e090245a8163761263deeb3355R94
There was a problem hiding this comment.
I see the test has been moved here https://github.com/ros/ros_comm/pull/1144/files#diff-719224e090245a8163761263deeb3355R91 (slightly different line than the above link. LGTM.
|
|
||
| import logging | ||
| import rosgraph.roslogging |
There was a problem hiding this comment.
Is there a reason for these imports to not be at module level?
Same below.
There was a problem hiding this comment.
Not that I know of... imports were already done in the method, I just did the same. I ll change them...
There was a problem hiding this comment.
This is still pending. Maybe you are still working on this. Please let me know when I should review the patch again.
There was a problem hiding this comment.
I actually missed these...
I also noticed the test_wait_for_service has a comment "lazy-import for coverage"... Is this something I should be concerned about ? I don't know how I can check coverage in this repo... any doc you could point me to, describing this ?
| rospy.logout('out', logger_name="child1") | ||
| rospy.logerr('err', logger_name="child1") | ||
| rospy.logfatal('fatal', logger_name="child1") | ||
|
|
There was a problem hiding this comment.
Please avoid empty lines after def ...(...): or try: (PEP 8).
asmodehn
force-pushed
the
rospy_logger_tests
branch
from
fb66356 to
d38c534
Compare
clients/rospy/src/rospy/core.py
Outdated
| if filename[0] == '_': | ||
| filename = filename[1:] | ||
| if filename == suffix: | ||
| raise rospy.exceptions.ROSException('invalid configure_logging parameter: %s'%node_name) |
There was a problem hiding this comment.
Please make sure that lines which haven't changed do not appear in the diff.
There was a problem hiding this comment.
ah sorry this happens because of my editor switching to unix endlines by default...
I ll put back the windows endlines there to get a proper diff.
|
|
||
| rosout_logger = logging.getLogger('rosout') | ||
|
|
||
| print("HANDLERS : " + str(rosout_logger.handlers)) |
There was a problem hiding this comment.
Can this debug output be removed?
| print("HANDLERS : " + str(rosout_logger.handlers)) | ||
| self.assertTrue(len(rosout_logger.handlers) == 2) | ||
| self.assertTrue(rosout_logger.handlers[0], rosgraph.roslogging.RosStreamHandler) | ||
| # self.assertTrue(rosout_logger.handlers[1], rospy.impl.rosout.RosOutHandler) |
There was a problem hiding this comment.
Should this be removed too?
There was a problem hiding this comment.
I left it there to illustrate what the handlers should be. I might as well actually check it.
|
|
||
| import logging | ||
| import rosgraph.roslogging |
There was a problem hiding this comment.
This is still pending. Maybe you are still working on this. Please let me know when I should review the patch again.
asmodehn
force-pushed
the
rospy_logger_tests
branch
from
11ab850 to
7ee680d
Compare
|
I was actually thinking of leaving this PR like this (except minor changes), given that throttled and named loggers are already tested in |
| @@ -39,9 +39,14 @@ | |||
| import unittest | |||
| import time | |||
| import random | |||
| from StringIO import StringIO | |||
There was a problem hiding this comment.
This doesn't work with Python 3. Please use the same conditional import as in the other files.
| rospy.core.configure_logging("/") | ||
| self.fail("configure_logging should not accept a the root namespace as the node_name param") | ||
| except: pass | ||
| rospy.core.configure_logging("/node/name") |
There was a problem hiding this comment.
I see the test has been moved here https://github.com/ros/ros_comm/pull/1144/files#diff-719224e090245a8163761263deeb3355R91 (slightly different line than the above link. LGTM.
|
@asmodehn Thank you! |
now allowing to customize out/err streams for RosStreamHandler.
This is still WIP to get feedback, and we should add test for named loggers.
Throttled loggers need node to be initialized (to get the time...) so we might test them in a rostest instead (unless we can somehow remove that constraint)
FYI : I also found a mix of unix/windows endlines in rospy.core... not addressing this issue here.