improved rospy core logger test. by asmodehn · Pull Request #1144 · ros/ros_comm

asmodehn · 2017-08-16T08:27:53Z

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.

now allowing to customize out/err streams for RosStreamHandler

asmodehn · 2017-08-17T02:53:29Z

I also modified the test_rospy_client_online to check logs without relying on changing values of sys.stdout (that can be modified by anybody, anywhere, for any other reason).
This test also checks named and throttled loggers.

Guideline : keep the core as pure (functional) as possible, push side-effects to the side.

dirk-thomas

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.

dirk-thomas · 2017-08-17T17:18:40Z

test/test_rospy/test/unit/test_rospy_core.py

-            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")


Why has this test been removed?

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

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.

dirk-thomas · 2017-08-17T17:26:43Z

test/test_rospy/test/rostest/test_rospy_client_online.py

-        real_stderr = sys.stderr

+        import logging
+        import rosgraph.roslogging


Is there a reason for these imports to not be at module level?

Same below.

Not that I know of... imports were already done in the method, I just did the same. I ll change them...

This is still pending. Maybe you are still working on this. Please let me know when I should review the patch again.

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 ?

dirk-thomas · 2017-08-17T17:27:21Z

test/test_rospy/test/unit/test_rospy_core.py

-        rospy.logout('out', logger_name="child1")
-        rospy.logerr('err', logger_name="child1")
-        rospy.logfatal('fatal', logger_name="child1")
+


Please avoid empty lines after def ...(...): or try: (PEP 8).

dirk-thomas · 2017-08-22T16:20:39Z

clients/rospy/src/rospy/core.py

+        if filename[0] == '_':
+            filename = filename[1:]
+        if filename == suffix:
+            raise rospy.exceptions.ROSException('invalid configure_logging parameter: %s'%node_name)


Please make sure that lines which haven't changed do not appear in the diff.

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.

dirk-thomas · 2017-08-22T16:23:19Z

test/test_rospy/test/rostest/test_rospy_client_online.py

+
+        rosout_logger = logging.getLogger('rosout')
+
+        print("HANDLERS : " + str(rosout_logger.handlers))


Can this debug output be removed?

dirk-thomas · 2017-08-22T16:23:29Z

test/test_rospy/test/rostest/test_rospy_client_online.py

+        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)


Should this be removed too?

I left it there to illustrate what the handlers should be. I might as well actually check it.

dirk-thomas · 2017-08-22T16:25:22Z

test/test_rospy/test/rostest/test_rospy_client_online.py

-        real_stderr = sys.stderr

+        import logging
+        import rosgraph.roslogging


This is still pending. Maybe you are still working on this. Please let me know when I should review the patch again.

asmodehn · 2017-08-23T09:06:08Z

I was actually thinking of leaving this PR like this (except minor changes), given that throttled and named loggers are already tested in test_rospy_client_online.py.
But let me know if you are aware of any related change that should be made in this PR, I can add them.

dirk-thomas

Hopefully the last comment. Thanks for iterating on this.

dirk-thomas · 2017-08-23T18:33:21Z

test/test_rospy/test/unit/test_rospy_core.py

 import unittest
 import time
 import random
+from StringIO import StringIO


This doesn't work with Python 3. Please use the same conditional import as in the other files.

dirk-thomas · 2017-08-23T18:58:26Z

test/test_rospy/test/unit/test_rospy_core.py

-            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")


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.

dirk-thomas · 2017-08-28T17:08:07Z

@asmodehn Thank you!

improved rospy core logger test.

272f56e

now allowing to customize out/err streams for RosStreamHandler

asmodehn mentioned this pull request Aug 16, 2017

[rospy] Write log for class method with class name for rospy #1043

Merged

asmodehn added 2 commits August 17, 2017 11:02

cleaner RosStreamHandler API to not break bwcompat

646cafd

testing rospy logger without relying on global state magic.

93448e0

fixing rosgraph test_roslogging to not play with sys.stdout dynamically.

83dbf80

dirk-thomas reviewed Aug 17, 2017

View reviewed changes

dirk-thomas added the enhancement label Aug 17, 2017

cleaning empty lines and imports in test_loggers method

d38c534

asmodehn force-pushed the rospy_logger_tests branch from fb66356 to d38c534 Compare August 18, 2017 09:33

dirk-thomas reviewed Aug 22, 2017

View reviewed changes

restore windows endlines. cleanup imports and debug prints.

7ee680d

asmodehn force-pushed the rospy_logger_tests branch from 11ab850 to 7ee680d Compare August 23, 2017 09:06

dirk-thomas reviewed Aug 23, 2017

View reviewed changes

fixing stringio import

4c2523b

dirk-thomas merged commit 44bbbd1 into ros:lunar-devel Aug 28, 2017

dirk-thomas mentioned this pull request Oct 26, 2017

changes between 1.12.7 and 1.13.4 for backporting #1205

Merged


		rosout_logger = logging.getLogger('rosout')

		print("HANDLERS : " + str(rosout_logger.handlers))

Conversation

asmodehn commented Aug 16, 2017

Uh oh!

asmodehn commented Aug 17, 2017

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmodehn commented Aug 23, 2017

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Aug 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants