add more tests and fixes for command inheritance

- fix CommandType.__repr__
- secop/modules.py: command properties are allowed to be configured:
   - section 2: remove comment and rename
   - section 3: all accessible properties should be checked
- command description should be inherited also when taken from docstring
- move test for command inheritance to test_modules.py
- added tests to check for valid properties of commands

Change-Id: I5fd04e03be1faec5e54fed9735620bc5dc0f89c0

secop/datatypes.py

@@ -955,10 +955,9 @@ class CommandType(DataType):
         return props
 
     def __repr__(self):
-        argstr = repr(self.argument) if self.argument else ''
         if self.result is None:
-            return 'CommandType(%s)' % argstr
+            return 'CommandType(%s)' % (repr(self.argument) if self.argument else '')
-        return 'CommandType(%s, %s)' % (argstr, repr(self.result))
+        return 'CommandType(%s, %s)' % (repr(self.argument), repr(self.result))
 
     def __call__(self, value):
         """return the validated argument value or raise"""

secop/modules.py

@@ -318,26 +318,23 @@ class Module(HasAccessibles):
 
         # 2) check and apply parameter_properties
         #    specified as '<paramname>.<propertyname> = <propertyvalue>'
+        #    this may also be done on commands: e.g. 'stop.visibility = advanced'
         for k, v in list(cfgdict.items()):  # keep list() as dict may change during iter
             if '.' in k[1:]:
-                paramname, propname = k.split('.', 1)
+                aname, propname = k.split('.', 1)
                 propvalue = cfgdict.pop(k)
-                paramobj = self.accessibles.get(paramname, None)
+                aobj = self.accessibles.get(aname, None)
-                # paramobj might also be a command (not sure if this is needed)
+                if aobj:
-                if paramobj:
-                    # no longer needed, this conversion is done by DataTypeType.__call__:
-                    # if propname == 'datatype':
-                    #     propvalue = get_datatype(propvalue, k)
                     try:
-                        paramobj.setProperty(propname, propvalue)
+                        aobj.setProperty(propname, propvalue)
                     except KeyError:
                         errors.append("'%s.%s' does not exist" %
-                                      (paramname, propname))
+                                      (aname, propname))
                     except BadValueError as e:
                         errors.append('%s.%s: %s' %
-                                      (paramname, propname, str(e)))
+                                      (aname, propname, str(e)))
                 else:
-                    errors.append('%r not found' % paramname)
+                    errors.append('%r not found' % aname)
 
         # 3) check config for problems:
         #    only accept remaining config items specified in parameters
@@ -423,11 +420,11 @@ class Module(HasAccessibles):
                 self.checkProperties()
             except ConfigError as e:
                 errors.append(str(e))
-            for pname, p in self.parameters.items():
+            for aname, aobj in self.accessibles.items():
                 try:
-                    p.checkProperties()
+                    aobj.checkProperties()
-                except ConfigError as e:
+                except (ConfigError, ProgrammingError) as e:
-                    errors.append('%s: %s' % (pname, e))
+                    errors.append('%s: %s' % (aname, e))
         if errors:
             raise ConfigError(errors)
 

secop/params.py

@@ -346,6 +346,9 @@ class Command(Accessible):
 
     def __init__(self, argument=False, *, result=None, inherit=True, **kwds):
         super().__init__()
+        if 'datatype' in kwds:
+            # self.init will complain about invalid keywords except 'datatype', as this is a property
+            raise ProgrammingError("Command() got an invalid keyword 'datatype'")
         self.init(kwds)
         if result or kwds or isinstance(argument, DataType) or not callable(argument):
             # normal case
@@ -362,7 +365,7 @@ class Command(Accessible):
             self.func = argument  # this is the wrapped method!
             if argument.__doc__:
                 self.description = inspect.cleandoc(argument.__doc__)
-            self.name = self.func.__name__
+            self.name = self.func.__name__  # this is probably not needed
         self._inherit = inherit  # save for __set_name__
 
     def __set_name__(self, owner, name):
@@ -397,6 +400,7 @@ class Command(Accessible):
         """called when used as decorator"""
         if 'description' not in self.propertyValues and func.__doc__:
             self.description = inspect.cleandoc(func.__doc__)
+            self.ownProperties['description'] = self.description
         self.func = func
         return self
 

test/test_modules.py

@@ -26,7 +26,7 @@ import threading
 
 import pytest
 
-from secop.datatypes import BoolType, FloatRange, StringType, IntRange, CommandType
+from secop.datatypes import BoolType, FloatRange, StringType, IntRange
 from secop.errors import ProgrammingError, ConfigError
 from secop.modules import Communicator, Drivable, Readable, Module
 from secop.params import Command, Parameter
@@ -34,7 +34,11 @@ from secop.poller import BasicPoller
 
 
 class DispatcherStub:
-    OMIT_UNCHANGED_WITHIN = 0
+    # the first update from the poller comes a very short time after the
+    # initial value from the timestamp. However, in the test below
+    # the second update happens after the updates dict is cleared
+    # -> we have to inhibit the 'omit unchanged update' feature
+    omit_unchanged_within = 0
 
     def __init__(self, updates):
         self.updates = updates
@@ -260,9 +264,101 @@ def test_param_inheritance():
         Base('o', logger, {'description': ''}, srv)
 
 
-def test_mixin():
+def test_command_inheritance():
-    # srv = ServerStub({})
+    class Base(Module):
+        @Command(BoolType(), visibility=2)
+        def cmd(self, arg):
+            """base"""
 
+    class Sub1(Base):
+        @Command(group='grp')
+        def cmd(self, arg):
+            """first"""
+
+    class Sub2(Sub1):
+        @Command(None, result=BoolType())
+        def cmd(self):  # pylint: disable=arguments-differ
+            """second"""
+
+    class Sub3(Base):
+        # when either argument or result is given, the other one is assumed to be None
+        # i.e. here we override the argument with None
+        @Command(result=FloatRange())
+        def cmd(self, arg):
+            """third"""
+
+    assert Sub1.accessibles['cmd'].for_export() == {
+        'description': 'first', 'group': 'grp', 'visibility': 2,
+        'datainfo': {'type': 'command', 'argument': {'type': 'bool'}}
+    }
+
+    assert Sub2.accessibles['cmd'].for_export() == {
+        'description': 'second', 'group': 'grp', 'visibility': 2,
+        'datainfo': {'type': 'command', 'result': {'type': 'bool'}}
+    }
+
+    assert Sub3.accessibles['cmd'].for_export() == {
+        'description': 'third', 'visibility': 2,
+        'datainfo': {'type': 'command', 'result': {'type': 'double'}}
+    }
+
+    for cls in locals().values():
+        if hasattr(cls, 'accessibles'):
+            for p in cls.accessibles.values():
+                assert isinstance(p.ownProperties, dict)
+                assert p.copy().ownProperties == {}
+
+
+def test_command_check():
+    srv = ServerStub({})
+
+    class Good(Module):
+        @Command(description='available')
+        def with_description(self):
+            pass
+        @Command()
+        def with_docstring(self):
+            """docstring"""
+
+    Good('o', logger, {'description': ''}, srv)
+
+    class Bad1(Module):
+        @Command
+        def without_description(self):
+            pass
+
+    class Bad2(Module):
+        @Command()
+        def without_description(self):
+            pass
+
+    for cls in Bad1, Bad2:
+        with pytest.raises(ConfigError) as e_info:
+            cls('o', logger, {'description': ''}, srv)
+        assert 'description' in repr(e_info.value)
+
+    class BadDatatype(Module):
+        @Command(FloatRange(0.1, 0.9), result=FloatRange())
+        def cmd(self):
+            """valid command"""
+
+    BadDatatype('o', logger, {'description': ''}, srv)
+
+    # test for command property checking
+    with pytest.raises(ProgrammingError):
+        BadDatatype('o', logger, {
+            'description': '',
+            'cmd.argument': {'type': 'double', 'min': 1, 'max': 0},
+        }, srv)
+
+    with pytest.raises(ProgrammingError):
+        BadDatatype('o', logger, {
+            'description': '',
+            'cmd.visibility': 'invalid',
+        }, srv)
+
+
+def test_mixin():
     class Mixin:  # no need to inherit from Module or HasAccessible
         value = Parameter(unit='K')  # missing datatype and description acceptable in mixins
         param1 = Parameter('no datatype yet', fmtstr='%.5f')
@@ -276,7 +372,7 @@ def test_mixin():
         param1 = Parameter(datatype=FloatRange())
 
     with pytest.raises(ProgrammingError):
-        class MixedModule(Mixin):
+        class MixedModule(Mixin):  # pylint: disable=unused-variable
             param1 = Parameter('', FloatRange(), fmtstr=0)  # fmtstr must be a string
 
     assert repr(MixedDrivable.status.datatype) == repr(Drivable.status.datatype)
@@ -305,10 +401,29 @@ def test_mixin():
         }, srv)
 
 
+def test_override():
+    class Mod(Drivable):
+        value = 5  # overriding the default value
+
+        def stop(self):
+            """no decorator needed"""
+
+    assert Mod.value.default == 5
+    assert Mod.stop.description == "no decorator needed"
+
+    class Mod2(Drivable):
+        @Command()
+        def stop(self):
+            pass
+
+    assert Mod2.stop.description == Drivable.stop.description
+
+
 def test_command_config():
     class Mod(Module):
         @Command(IntRange(0, 1), result=IntRange(0, 1))
         def convert(self, value):
+            """dummy conversion"""
             return value
 
     srv = ServerStub({})
@@ -332,3 +447,15 @@ def test_command_config():
         'result': {'type': 'bool'},
     }
 
+
+def test_command_none():
+    srv = ServerStub({})
+
+    class Mod(Drivable):
+        pass
+
+    class Mod2(Drivable):
+        stop = None
+
+    assert 'stop' in Mod('o', logger, {'description': ''}, srv).accessibles
+    assert 'stop' not in Mod2('o', logger, {'description': ''}, srv).accessibles

test/test_params.py

@@ -98,6 +98,12 @@ def test_Override():
     Mod.p1.default = False
     assert repr(Mod.p1) == repr(Base.p1)
 
+    for cls in locals().values():
+        if hasattr(cls, 'accessibles'):
+            for p in cls.accessibles.values():
+                assert isinstance(p.ownProperties, dict)
+                assert p.copy().ownProperties == {}
+
 
 def test_Export():
     class Mod(HasAccessibles):