Revert 'composite attrs done last' policy

This was the wrong way to solve the issue of applying a log directory to
underlying log file argument flags. Drop the apply-composite-attrs-last
policy and instead add an explicit method for enabling logging in each
command/agent. The scenario will enable logging inside `prepare_agent()`
but agents will not by default. Add unit tests to match.

pysipp/agent.py

@@ -86,11 +86,6 @@ def iter_logfile_items(self):
     def iter_toconsole_items(self):
         yield 'screen_file', self.screen_file
 
-    def enable_tracing(self):
-        for name in self._log_types:
-            attr_name = 'trace_' + name
-            setattr(self, attr_name, True)
-
     @property
     def cmd(self):
         """Rendered SIPp command string
@@ -104,13 +99,6 @@ def logdir(self):
     @logdir.setter
     def logdir(self, dirpath):
         assert path.isdir(dirpath), '{} is an invalid path'.format(dirpath)
-        for name, attr in self.iter_logfile_items():
-            # assemble all log file paths
-            setattr(self, name,
-                    path.join(dirpath, "{}_{}".format(self.name, name)))
-
-        # enable all corresponding trace flag args
-        self.enable_tracing()
         self._logdir = dirpath
 
     @property
@@ -124,6 +112,27 @@ def plays_media(self, patt='play_pcap_audio'):
         with open(self.scen_file, 'r') as sf:
             return bool(re.search(patt, sf.read()))
 
+    def enable_tracing(self):
+        """Enable trace flags for this command
+        """
+        for name in self._log_types:
+            attr_name = 'trace_' + name
+            setattr(self, attr_name, True)
+
+    def enable_logging(self, logdir=None):
+        """Enable agent logging by appending appropriately named log file
+        arguments to the underlying command.
+        """
+        # prefix all log file paths
+        for name, attr in self.iter_logfile_items():
+            setattr(
+                self, name, path.join(
+                    logdir or self.logdir or tempfile.gettempdir(),
+                    "{}_{}".format(self.name, name))
+            )
+
+        self.enable_tracing()
+
 
 def path2namext(filepath):
     if not filepath:
@@ -346,9 +355,11 @@ def merge(dicts):
         ordered = [self._defaults, secondary, agent.todict()]
         for name, defs in zip(['defaults', dname, 'agent.todict()'], ordered):
             log.debug("'{}' contents:\n{}".format(name, defs))
+
         params = merge(ordered)
         log.debug("merged contents:\n{}".format(params))
         ua = UserAgent(defaults=params)
+        ua.enable_logging()
 
         # call post defaults hook
         plugin.mng.hook.pysipp_post_ua_defaults(ua=ua)

pysipp/command.py

@@ -146,18 +146,8 @@ def keys(cls):
 
         def applydict(self, d):
             """Apply contents of dict `d` onto local instance variables.
-            Composite attrs (those not found in `_specparams`) are applied
-            last so that any inter-attr dependencies are (hopefully) resolved.
             """
-            composite = OrderedDict()
             for name, value in d.items():
-                if name not in self._specparams:
-                    composite[name] = value
-                else:
-                    setattr(self, name, value)
-
-            # apply composites last
-            for name, value in composite.items():
                 setattr(self, name, value)
 
         def todict(self):

pysipp/report.py

@@ -34,7 +34,7 @@ def err_summary(agents2procs):
     if any(name2ec.values()):
         # raise a detailed error
         msg = "Some agents failed\n"
-        msg += '\n'.join("'{}' with exit code '{}' -> {}".format(
+        msg += '\n'.join("'{}' with exit code {} -> {}".format(
             name, rc, EXITCODES.get(rc, "unknown exit code"))
             for name, rc in name2ec.items()
         )

tests/test_agent.py

@@ -1,7 +1,10 @@
 '''
-Agent wrapping
+pysipp.agent module tests
 '''
 import pytest
+import tempfile
+import os
+import pysipp
 from pysipp import agent, launch, plugin
 
 
@@ -11,22 +14,56 @@ def ua():
 
 
 def test_ua(ua):
+    """Set up a typeless agent and perform basic attr checks
+    """
     sock = ('10.10.9.9', 5060)
     ua.proxyaddr = sock
     assert ua.name == str(None)
     assert "'{}':'{}'".format(*sock) in ua.render()
 
 
-def test_logdir(ua):
-    logdir = ua.logdir
+def check_log_files(ua, logdir=None):
+    logdir = logdir or ua.logdir
     assert logdir
+
+    # check attr values contain logdir and agent name
+    for name, path in ua.iter_logfile_items():
+        assert logdir in path
+        assert ua.name in path
+
     cmd = ua.render()
+    assert logdir in cmd
     logs = [token for token in cmd.split() if logdir in token]
-    assert len(logs) == len(ua._log_types)  # currently default num of logs
+    # check num of args with logdir in the value
+    assert len(logs) == len(ua._log_types)
+
+
+@pytest.mark.parametrize(
+    "funcname",
+    ['ua', 'client', 'server'],
+)
+def test_logdir(funcname):
+    """Verify that a default logdir is set and filenames are
+    based on the agent's name.
+    """
+    func = getattr(agent, funcname)
+    # enables SIPp logging by default
+    ua = agent.Scenario([func()]).prepare()[0]
+    check_log_files(ua, tempfile.gettempdir())
+
+
+def test_scen_logdir():
+    """Verify log file arguments when logdir is set using Scenario.defaults
+    """
+    scen = pysipp.scenario()
+    logdir = os.getcwd()
+    scen.defaults.logdir = logdir
+    for ua in scen.prepare():
+        check_log_files(ua, logdir)
 
 
 def test_client():
-    # built-in scen
+    # check the built-in uac xml scenario
     remote_sock = ('192.168.1.1', 5060)
     uac = agent.client(destaddr=remote_sock)
     cmdstr = uac.render()

tests/test_stack.py

@@ -25,10 +25,8 @@ class blockall(object):
         def pysipp_load_scendir(self, path, xmls, confpy):
             return False
 
-    ba = blockall()
-    pysipp.plugin.mng.register(ba)
-    assert not len(list(scenwalk()))
-    pysipp.plugin.mng.unregister(ba)
+    with pysipp.plugin.register([blockall()]):
+        assert not len(list(scenwalk()))
 
     @pysipp.plugin.hookimpl
     def confpy_included(self, path, xmls, confpy):
@@ -89,10 +87,8 @@ def test_unreachable_uas(basic_scen):
     # verify log file generation
     for ua in basic_scen.prepare():
         for name, path in ua.iter_logfile_items():
-            assert tempfile.gettempdir() in path
-            assert ua.name in path
-            assert logdir in path
             assert os.path.isfile(path)
+            os.remove(path)
 
 
 def test_hook_overrides(basic_scen):